Fix MockFileSystem.walk and add test.

Fix MockFileSystem.walk and add test.

BUG=616529
Committed: https://crrev.com/5fe63658bfaffc335caf48c43f13a8716297291b
Cr-Commit-Position: refs/heads/master@{#397593}

[raikiri, 2016-06-01 22:31:30]

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

[raikiri, 2016-06-01 23:03:39]

Style changes made

[qyearsley, 2016-06-01 23:08:44]

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

https://codereview.chromium.org/2029823002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem_mock.py:208:
FILE_SYSTEM_TUPLES = []
Non-global variable names use lowercase with underscore, e.g.
file_system_tuples.

https://codereview.chromium.org/2029823002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem_mock.py:230:
subdir = self.walk(dir)
This variable name could probably be improved; is it a tuple or a list of
tuples? The name `subdir` suggests that it may be a string.

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

https://codereview.chromium.org/2029823002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem_mock_walk_unittest.py:40:
'foo/c': ''}
These don't have to be globals now; they could be moved into the one test method
that uses them. (And they don't have to be assigned variable names; they can be
passed as literals to the functions that take them.)

Other test methods will likely want to use different file mappings.

Good test methods generally:

 - are simple and flat
 - contain literal values that are easy to understand
 - can be understood by just looking at that one test method

https://codereview.chromium.org/2029823002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem_mock_walk_unittest.py:95:
def test_filesystem_walk(self):
I'm not quite sure whether this walk method will work with directories that are
more deeply nested, e.g.:

foo
├── bar
│   └── baz
│        └── quux
├── a
│   └── x
│       └── y
│           └── z
├── b
└── c

I think it would be helpful to add a separate test method that tests a more
deeply nested directory structure, to check that this "walk" is walking through
the directories recursively to an arbitrary depth.

https://codereview.chromium.org/2029823002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem_mock_walk_unittest.py:98:
host.filesystem.walk(MOCK_DIR)
self.assertEqual(
    [
        ('foo', ['bar'], ['a', 'b', 'c']),
        ('foo/bar', [], ['baz']),
    ]
    host.filesystem.walk(MOCK_DIR))

https://codereview.chromium.org/2029823002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem_mock_walk_unittest.py:99:
self.assertTrue(False)
This line should be removed.

[raikiri, 2016-06-01 23:37:55]

Added deep folder test

[qyearsley, 2016-06-02 00:07:15]

https://codereview.chromium.org/2029823002/diff/60001/third_party/WebKit/Tool...
File
third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem_mock_walk_unittest.py
(right):

https://codereview.chromium.org/2029823002/diff/60001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem_mock_walk_unittest.py:1:
# Copyright (C) 2011 Google Inc. All rights reserved.
The new test method should be added in filesystem_mock_unittest.py rather than
being in a new file -- in general, for every non-test file "foo.py", there
should be one unit test file "foo_unittest.py".

https://codereview.chromium.org/2029823002/diff/60001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem_mock_walk_unittest.py:110:
self.assertEquals(host.filesystem.walk(mock_dir), [('foo', ['a', 'bar'], ['c',
'b']), ('foo/a', ['z'], ['x', 'y']), ('foo/a/z', [], ['lyrics']), ('foo/bar',
[], ['quux', 'baz'])])
Style nit: this line is a bit long, and could potentially be made easier-to-read
by breaking it up, e.g.
self.assertEquals(
    host.filesystem.walk(mock_dir),
    [('foo', ['a', 'bar'], ['c', 'b']),
     ('foo/a', ['z'], ['x', 'y']),
     ('foo/a/z', [], ['lyrics']),
     ('foo/bar', [], ['quux', 'baz'])])

https://codereview.chromium.org/2029823002/diff/60001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer_unittest.py
(right):

https://codereview.chromium.org/2029823002/diff/60001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer_unittest.py:57:
"dry_run": False,
This change is not related to this particular fix, so it can be taken out of
this CL.

Note: one way to undo changes to a file in a branch is to run: `git checkout
master <filename>`

[qyearsley, 2016-06-02 14:24:38]

It looks like the changes for http://crbug.com/584660 (skipping OWNERS file) got
accidentlly added to this changelist, which was originally for the
FileSystem.walk issue, right?

https://codereview.chromium.org/2029823002/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/W3CImportExpectations (right):

https://codereview.chromium.org/2029823002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/W3CImportExpectations:338:
#imported/wpt/quirks-mode/OWNERS [ Pass ]
This whole section can just be deleted from the file.

The reason why some lines have been commented out and have an "Owner" comment
listed above is to indicate who is responsible for which tests that are
imported.

https://codereview.chromium.org/2029823002/diff/120001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_OWNERS_unittest.py
(right):

https://codereview.chromium.org/2029823002/diff/120001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_OWNERS_unittest.py:1: #
Copyright (C) 2013 Adobe Systems Incorporated. All rights reserved.
These tests should go in test_importer_unittest.py.

https://codereview.chromium.org/2029823002/diff/120001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_OWNERS_unittest.py:44:
'/blink/w3c/empty_dir/OWNERS': '',
It's slightly confusing that there's a non-empty dir (in the fake list of files)
called empty_dir.

https://codereview.chromium.org/2029823002/diff/120001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_OWNERS_unittest.py:74:
'total_tests': 0, }])
Formatting nit: line 70 is quite long, and line 74 doesn't need a trailing comma
and space.

[raikiri, 2016-06-02 18:31:51]

I separated the CLs however the rebased master had test_importer.py changed.
This CL covers the fix to filesystem_mock.py

[qyearsley, 2016-06-02 20:57:03]

The bug link contains enough context for this change, but you could also make
the CL description clearer by being more specific: "Fix MockFileSystem.walk and
add test."

https://codereview.chromium.org/2029823002/diff/160001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py (right):

https://codereview.chromium.org/2029823002/diff/160001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py:1: # Copyright
(C) 2013 Adobe Systems Incorporated. All rights reserved.
This file should be removed from the CL (by updating master branch and replacing
this file with the latest version on master. Note, `git checkout master
w3c/test_importer.py` will get the master branch's copy of this file.

[raikiri, 2016-06-02 21:27:21]

Patchset #9 (id:160001) has been deleted

[raikiri, 2016-06-02 21:38:32]

Description was changed from

==========
Fix file system walk and add test.

BUG=616529
==========

to

==========
Fix MockFileSystem.walk and add test.

BUG=616529
==========

[raikiri, 2016-06-02 21:39:09]

Fixed files that were uploaded

[qyearsley, 2016-06-02 22:11:36]

Excellent, LGTM

[raikiri, 2016-06-02 22:12:28]

The CQ bit was checked by raikiri@google.com

[commit-bot: I haz the power, 2016-06-02 22:13:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2029823002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2029823002/180001

[commit-bot: I haz the power, 2016-06-02 23:36:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-02 23:36:35]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[qyearsley, 2016-06-02 23:53:42]

On 2016/06/02 at 23:36:35, commit-bot wrote:
> Try jobs failed on following builders:
>   mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

Looks like this change to MockFileSystem is changing what happens in the test
importer test, so it appears we *do* actually need to include a change to that
test in this CL:


From
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...:

[1258/1269]
webkitpy.w3c.test_importer_unittest.TestImporterTest.test_import_dir_with_no_tests
failed unexpectedly:
  Traceback (most recent call last):
    File
"/b/build/slave/linux/build/src/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer_unittest.py",
line 72, in test_import_dir_with_no_tests
      importer.do_import()
    File
"/b/build/slave/linux/build/src/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py",
line 174, in do_import
      self.import_tests()
    File
"/b/build/slave/linux/build/src/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py",
line 395, in import_tests
      if not self.import_in_place and not self.options.dry_run:
  AttributeError: Values instance has no attribute 'dry_run'

[raikiri, 2016-06-03 00:44:13]

The CQ bit was checked by raikiri@google.com

[raikiri, 2016-06-03 00:44:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
qyearsley@chromium.org
Link to the patchset: https://codereview.chromium.org/2029823002/#ps200001
(title: "set dry_run to False")

[commit-bot: I haz the power, 2016-06-03 00:45:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2029823002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2029823002/200001

[commit-bot: I haz the power, 2016-06-03 02:40:04]

Description was changed from

==========
Fix MockFileSystem.walk and add test.

BUG=616529
==========

to

==========
Fix MockFileSystem.walk and add test.

BUG=616529
==========

[commit-bot: I haz the power, 2016-06-03 02:40:05]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2016-06-03 02:45:32]

Description was changed from

==========
Fix MockFileSystem.walk and add test.

BUG=616529
==========

to

==========
Fix MockFileSystem.walk and add test.

BUG=616529

Committed: https://crrev.com/5fe63658bfaffc335caf48c43f13a8716297291b
Cr-Commit-Position: refs/heads/master@{#397593}
==========

[commit-bot: I haz the power, 2016-06-03 02:45:34]

Patchset 10 (id:??) landed as
https://crrev.com/5fe63658bfaffc335caf48c43f13a8716297291b
Cr-Commit-Position: refs/heads/master@{#397593}