Add file permissions to filesystem_mock

Add file permissions to filesystem_mock

BUG=574461
Committed: https://crrev.com/855b1c7ae159d928843ceacd5bd2313915115580
Cr-Commit-Position: refs/heads/master@{#400261}

[raikiri, 2016-06-14 19:48:29]

raikiri@google.com changed reviewers:
+ qyearsley@chromium.org

[Dirk Pranke, 2016-06-14 21:02:55]

dpranke@chromium.org changed reviewers:
+ dpranke@chromium.org

[Dirk Pranke, 2016-06-14 21:02:56]

What do we need this functionality for?

[qyearsley, 2016-06-14 21:36:05]

On 2016/06/14 at 21:02:56, dpranke wrote:
> What do we need this functionality for?

For testing a fix to bug http://crbug.com/574461 -- we were thinking of fixing
that problem by making changing the permissions to executable on copied files
after copying if the source file was executable.

The purpose of this patch would be for the unit tests for that change; what do
you think?

Slightly relatedly, do you think it may be worth it to start using mock
(http://www.voidspace.org.uk/python/mock/) in unit tests?

[qyearsley, 2016-06-14 21:36:30]

Description was changed from

==========
Added Fie permissions to filesystem_mock
BUG=
==========

to

==========
Added file permissions to filesystem_mock

BUG=
==========

[qyearsley, 2016-06-14 21:47:01]

qyearsley@chromium.org changed reviewers:
- dpranke@chromium.org

[qyearsley, 2016-06-14 21:47:02]

https://codereview.chromium.org/2066963002/diff/1/third_party/WebKit/Tools/Sc...
File third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem_mock.py
(right):

https://codereview.chromium.org/2066963002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem_mock.py:52:
self.perms = {}
Although it's nice that `perms` is the same length as `files`, I feel that it is
often nice to avoid abbreviations because:
 (1) if there are a lot of abbreviations in the code, one has to think about
whether the name is "perms" or "permissions", "cmds" or "commands", etc.
 (2) un-abbreviated variable names are usually less ambiguous.

That said, there are lots of other abbreviations in this code now, and perms is
also fine.

https://codereview.chromium.org/2066963002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem_mock.py:93:
self.perms[file_path] = temp_perms
List comprehensions (and related expressions) are awesome and allow you to
express some things more concisely in Python. For example:

mode_digits = [int(d) for d in mode]
self.perms[file_path] = '-' + ''.join(self.chmod_options(d) for d in
mode_digits)

https://codereview.chromium.org/2066963002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem_mock.py:95:
def chmod_options(self, arg):
1. The variable name `arg` could perhaps be improved.

A recent episode of TotT
(https://big.corp.google.com/~jmcmaster/testing/2016/06/episode-431-identifier...)
said that names should be two things: clear (know what it refers to) and precise
(know what it does not refer to). In this case, the name "arg" doesn't tell you
much about what it is.

Do you think `octal_digit` or `mode_digit` might make sense? Maybe there's
another better name?

2. The function name chmod_options doesn't tell me much about what this function
does -- maybe mode_digit_to_string? But that's a bit too long.

3. Since this function doesn't use `self`, you can put a `@staticmethod`
decorator above the def line, and remove the self argument.

https://codereview.chromium.org/2066963002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem_mock.py:106:
return switcher.get(arg, None)
None is the default default for dict.get, so this line could be changed to:
`return switcher.get(arg)`

https://codereview.chromium.org/2066963002/diff/1/third_party/WebKit/Tools/Sc...
File
third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem_mock_unittest.py
(right):

https://codereview.chromium.org/2066963002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem_mock_unittest.py:118:
path2 = 'bar'
It would be better to put these strings literals directly where they are used.

Also, names like `path2` are a signal that the thing probably doesn't need a
name.

https://codereview.chromium.org/2066963002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem_mock_unittest.py:120:
'bar': ''}
Formatting nit: This fits on one line pretty easily, and I think it's just as
easy to read if it's on one line.

[Dirk Pranke, 2016-06-14 22:34:58]

On 2016/06/14 21:36:05, qyearsley wrote:
> On 2016/06/14 at 21:02:56, dpranke wrote:
> > What do we need this functionality for?
> 
> For testing a fix to bug http://crbug.com/574461 -- we were thinking of fixing
> that problem by making changing the permissions to executable on copied files
> after copying if the source file was executable.

Ah, okay. That seems reasonable. However, the chmod permissions are terribly
unix-y,
and fancier than we seem likely to need, and I try to keep the filesystem API
very simple
and minimal. 

I suggest a more generic method like  'make_executable(bool)'.

> 
> The purpose of this patch would be for the unit tests for that change; what do
> you think?
> 
> Slightly relatedly, do you think it may be worth it to start using mock
> (http://www.voidspace.org.uk/python/mock/) in unit tests?

IMO using fakes like this are better than mocks pretty much every time, and I
try to avoid mocks like the plague.

See, e.g.

http://googletesting.blogspot.com/2013/05/testing-on-toilet-dont-overuse-mock...
http://martinfowler.com/articles/mocksArentStubs.html

for some links that share my view.

[qyearsley, 2016-06-15 18:34:07]

In this same CL, could you add the methods make_executable and is_executable to
FileSystem? (make_executable should use os.chmod, and FileSystem.is_executable
should use os.access).

Also, you could add more context in the CL description, and note that this is
for a fix to http://crbug.com/574461.

[qyearsley, 2016-06-15 18:35:39]

https://codereview.chromium.org/2066963002/diff/40001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem_mock.py
(right):

https://codereview.chromium.org/2066963002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem_mock.py:60:
while f not in self.permissions:
while -> if

https://codereview.chromium.org/2066963002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem_mock.py:81:
def chmod(self, file_path, mode):
If we're changing the public API of the FileSystem class (the public methods) to
include make_executable and is_executable, then we don't need chmod.

[qyearsley, 2016-06-15 21:54:25]

https://codereview.chromium.org/2066963002/diff/100001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem.py
(right):

https://codereview.chromium.org/2066963002/diff/100001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem.py:280:
os.chmod(file_path, stat.S_IXUSR)
Not sure if it makes a difference, but currently executable files in the
chromium repo on my workstation appear to have permissions -rwxr-x--- (mode 750)

So perhaps we may want to use:

  os.chmod(file_path, stat.S_IXUSR | stat.S_IXGRP)

https://codereview.chromium.org/2066963002/diff/100001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem_mock.py
(right):

https://codereview.chromium.org/2066963002/diff/100001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem_mock.py:62:
for f in self.files:
Depending on how is_executable is implemented, this may be unnecessary.

For example, if is_executable is:

    def is_executable(self, path):
        return self.permissions.get(path, False)

In this case, self.permissions is just a dict of paths to True for all paths
where the file is executable -- which behaves just the same as a set. In Python,
sets are also a basic type (like lists, tuples and dicts), so you could also do
this:

    def __init__(...):
        ...
        self.executable_files = set()
        ...

    def is_executable(path):
        return path in self.executable_files

https://codereview.chromium.org/2066963002/diff/100001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem_mock.py:81:
def chmod(self, file_path, mode):
In the MockFileSystem class, we still want to have is_exectuable and
make_executable (and not chmod), since the API of the mock class should look
just like that of the real class.

https://codereview.chromium.org/2066963002/diff/100001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem_mock.py:82: #
To be used to fix http://crbug.com/574461
This kind of thing might be more useful to put in the CL description, to answer
the question: "why is this CL being uploaded now?" If this is committed and then
later the way code is used changes, then the comment would be out of sync with
reality. (This would be "comment rot").

https://codereview.chromium.org/2066963002/diff/100001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem_mock.py:84:
mode = mode
Note, this statement does nothing.

https://codereview.chromium.org/2066963002/diff/100001/third_party/WebKit/Too...
File
third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem_mock_unittest.py
(right):

https://codereview.chromium.org/2066963002/diff/100001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem_mock_unittest.py:122:
self.assertEquals(host.filesystem.permissions, {'bar': False, 'foo': True})
Instead of a test for chmod, we would want to have a test for is_executable and
make_executable here.

[qyearsley, 2016-06-16 20:15:58]

Description was changed from

==========
Added file permissions to filesystem_mock

BUG=
==========

to

==========
Add file permissions to filesystem_mock

BUG=574461
==========

[qyearsley, 2016-06-16 20:16:08]

The CQ bit was checked by qyearsley@chromium.org

[qyearsley, 2016-06-16 20:16:09]

lgtm

[commit-bot: I haz the power, 2016-06-16 20:17:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2066963002/120001

[commit-bot: I haz the power, 2016-06-16 21:36:36]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-06-16 21:38:10]

Description was changed from

==========
Add file permissions to filesystem_mock

BUG=574461
==========

to

==========
Add file permissions to filesystem_mock

BUG=574461

Committed: https://crrev.com/855b1c7ae159d928843ceacd5bd2313915115580
Cr-Commit-Position: refs/heads/master@{#400261}
==========

[commit-bot: I haz the power, 2016-06-16 21:38:11]

Patchset 7 (id:??) landed as
https://crrev.com/855b1c7ae159d928843ceacd5bd2313915115580
Cr-Commit-Position: refs/heads/master@{#400261}