Regenerate MANIFEST.json when WPT tests are run

Regenerate MANIFEST.json when WPT tests are run

BUG=666957
R=qyearsley@chromium.org

[jeffcarp, 2017-01-19 23:41:39]

I haven't tested this out with a new test yet. Let me know if this is what you
had in mind.

[qyearsley, 2017-01-19 23:56:20]

Yeah, I think this is what we want :-D

I was kind of concerned about whether this would add too much overhead to
run-webkit-tests, but now that I've tried it out after adding a new test, I've
found that on my system it takes about 0.5 seconds -- which I think should be
OK.

But there's one issue with this approach as-is: it modifies the filesystem,
which means that after running run-webkit-tests, you're left with unexplained
local unstaged changes to MANIFEST.json.

Maybe in _wpt_manifest we probably want a save and cleanup step where we save
away the original MANIFEST.json (which might also have had local changes), and
then restore it after reading the modified version. What do you think?

https://codereview.chromium.org/2644783003/diff/1/third_party/WebKit/Tools/Sc...
File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py
(right):

https://codereview.chromium.org/2644783003/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py:741: def
_generate_manifest(self, dest_path):
I usually like to put helper methods under the methods that use them, because in
general this leads to a sequence of methods where higher-level bigger-picture
methods come first.

Here, since dest_path is only ever going to be the wpt directory, you could
remove that argument and have a variable
  wpt_path = self._webkit_finder.path_from_webkit_base('LayoutTests',
'external', 'wpt')

Then you could remove the logic for "css" below.

https://codereview.chromium.org/2644783003/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py:753: #
Regenerate manifest
Not sure if this comment is necessary, since the method name generate_manifest
should make this clear enough.

https://codereview.chromium.org/2644783003/diff/1/third_party/WebKit/Tools/Sc...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py
(right):

https://codereview.chromium.org/2644783003/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py:281:
# Assert manifest was regenerated once
Nit: period.

https://codereview.chromium.org/2644783003/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py:281:
# Assert manifest was regenerated once
Nit: period.

[jeffcarp, 2017-01-20 22:55:13]

On 2017/01/19 at 23:56:20, qyearsley wrote:
> Yeah, I think this is what we want :-D
> 
> I was kind of concerned about whether this would add too much overhead to
run-webkit-tests, but now that I've tried it out after adding a new test, I've
found that on my system it takes about 0.5 seconds -- which I think should be
OK.
> 
> But there's one issue with this approach as-is: it modifies the filesystem,
which means that after running run-webkit-tests, you're left with unexplained
local unstaged changes to MANIFEST.json.
> 
> Maybe in _wpt_manifest we probably want a save and cleanup step where we save
away the original MANIFEST.json (which might also have had local changes), and
then restore it after reading the modified version. What do you think?
> 
>
https://codereview.chromium.org/2644783003/diff/1/third_party/WebKit/Tools/Sc...
> File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py
(right):
> 
>
https://codereview.chromium.org/2644783003/diff/1/third_party/WebKit/Tools/Sc...
> third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py:741: def
_generate_manifest(self, dest_path):
> I usually like to put helper methods under the methods that use them, because
in general this leads to a sequence of methods where higher-level bigger-picture
methods come first.
> 
> Here, since dest_path is only ever going to be the wpt directory, you could
remove that argument and have a variable
>   wpt_path = self._webkit_finder.path_from_webkit_base('LayoutTests',
'external', 'wpt')
> 
> Then you could remove the logic for "css" below.
> 
>
https://codereview.chromium.org/2644783003/diff/1/third_party/WebKit/Tools/Sc...
> third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py:753: #
Regenerate manifest
> Not sure if this comment is necessary, since the method name generate_manifest
should make this clear enough.
> 
>
https://codereview.chromium.org/2644783003/diff/1/third_party/WebKit/Tools/Sc...
> File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py
(right):
> 
>
https://codereview.chromium.org/2644783003/diff/1/third_party/WebKit/Tools/Sc...
>
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py:281:
# Assert manifest was regenerated once
> Nit: period.
> 
>
https://codereview.chromium.org/2644783003/diff/1/third_party/WebKit/Tools/Sc...
>
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py:281:
# Assert manifest was regenerated once
> Nit: period.

Addressed CL feedback.

I'm not sure I understand that flow entirely. What ends up being responsible for
MANIFEST.json changes? If you add a new test, wouldn't you want the
MANIFEST.json to change with it?

[qyearsley, 2017-01-20 23:07:09]

On 2017/01/20 at 22:55:13, jeffcarp wrote:
> On 2017/01/19 at 23:56:20, qyearsley wrote:
> > Yeah, I think this is what we want :-D
> > 
> > I was kind of concerned about whether this would add too much overhead to
run-webkit-tests, but now that I've tried it out after adding a new test, I've
found that on my system it takes about 0.5 seconds -- which I think should be
OK.
> > 
> > But there's one issue with this approach as-is: it modifies the filesystem,
which means that after running run-webkit-tests, you're left with unexplained
local unstaged changes to MANIFEST.json.
> > 
> > Maybe in _wpt_manifest we probably want a save and cleanup step where we
save away the original MANIFEST.json (which might also have had local changes),
and then restore it after reading the modified version. What do you think?
> > 
> >
https://codereview.chromium.org/2644783003/diff/1/third_party/WebKit/Tools/Sc...
> > File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py
(right):
> > 
> >
https://codereview.chromium.org/2644783003/diff/1/third_party/WebKit/Tools/Sc...
> > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py:741: def
_generate_manifest(self, dest_path):
> > I usually like to put helper methods under the methods that use them,
because in general this leads to a sequence of methods where higher-level
bigger-picture methods come first.
> > 
> > Here, since dest_path is only ever going to be the wpt directory, you could
remove that argument and have a variable
> >   wpt_path = self._webkit_finder.path_from_webkit_base('LayoutTests',
'external', 'wpt')
> > 
> > Then you could remove the logic for "css" below.
> > 
> >
https://codereview.chromium.org/2644783003/diff/1/third_party/WebKit/Tools/Sc...
> > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py:753: #
Regenerate manifest
> > Not sure if this comment is necessary, since the method name
generate_manifest should make this clear enough.
> > 
> >
https://codereview.chromium.org/2644783003/diff/1/third_party/WebKit/Tools/Sc...
> > File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py
(right):
> > 
> >
https://codereview.chromium.org/2644783003/diff/1/third_party/WebKit/Tools/Sc...
> >
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py:281:
# Assert manifest was regenerated once
> > Nit: period.
> > 
> >
https://codereview.chromium.org/2644783003/diff/1/third_party/WebKit/Tools/Sc...
> >
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py:281:
# Assert manifest was regenerated once
> > Nit: period.
> 
> Addressed CL feedback.
> 
> I'm not sure I understand that flow entirely. What ends up being responsible
for MANIFEST.json changes? If you add a new test, wouldn't you want the
MANIFEST.json to change with it?

You would at some point want the MANIFEST.json to change with it, that's true. I
was just thinking that it might be confusing if you got changes after just
running a test, since currently you can expect that running the tests won't
change anything.

I was thinking that only the importer would be responsible for making changes to
MANIFEST.json.

The main concern is that we don't want the test runner to do anything unexpected
or surprising to the user (modifying files), but we also want it to behave the
way people expect (finding new tests).

[jeffcarp, 2017-01-23 21:21:42]

On 2017/01/20 at 23:07:09, qyearsley wrote:
> On 2017/01/20 at 22:55:13, jeffcarp wrote:
> > On 2017/01/19 at 23:56:20, qyearsley wrote:
> > > Yeah, I think this is what we want :-D
> > > 
> > > I was kind of concerned about whether this would add too much overhead to
run-webkit-tests, but now that I've tried it out after adding a new test, I've
found that on my system it takes about 0.5 seconds -- which I think should be
OK.
> > > 
> > > But there's one issue with this approach as-is: it modifies the
filesystem, which means that after running run-webkit-tests, you're left with
unexplained local unstaged changes to MANIFEST.json.
> > > 
> > > Maybe in _wpt_manifest we probably want a save and cleanup step where we
save away the original MANIFEST.json (which might also have had local changes),
and then restore it after reading the modified version. What do you think?
> > > 
> > >
https://codereview.chromium.org/2644783003/diff/1/third_party/WebKit/Tools/Sc...
> > > File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py
(right):
> > > 
> > >
https://codereview.chromium.org/2644783003/diff/1/third_party/WebKit/Tools/Sc...
> > > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py:741:
def _generate_manifest(self, dest_path):
> > > I usually like to put helper methods under the methods that use them,
because in general this leads to a sequence of methods where higher-level
bigger-picture methods come first.
> > > 
> > > Here, since dest_path is only ever going to be the wpt directory, you
could remove that argument and have a variable
> > >   wpt_path = self._webkit_finder.path_from_webkit_base('LayoutTests',
'external', 'wpt')
> > > 
> > > Then you could remove the logic for "css" below.
> > > 
> > >
https://codereview.chromium.org/2644783003/diff/1/third_party/WebKit/Tools/Sc...
> > > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py:753: #
Regenerate manifest
> > > Not sure if this comment is necessary, since the method name
generate_manifest should make this clear enough.
> > > 
> > >
https://codereview.chromium.org/2644783003/diff/1/third_party/WebKit/Tools/Sc...
> > > File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py
(right):
> > > 
> > >
https://codereview.chromium.org/2644783003/diff/1/third_party/WebKit/Tools/Sc...
> > >
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py:281:
# Assert manifest was regenerated once
> > > Nit: period.
> > > 
> > >
https://codereview.chromium.org/2644783003/diff/1/third_party/WebKit/Tools/Sc...
> > >
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py:281:
# Assert manifest was regenerated once
> > > Nit: period.
> > 
> > Addressed CL feedback.
> > 
> > I'm not sure I understand that flow entirely. What ends up being responsible
for MANIFEST.json changes? If you add a new test, wouldn't you want the
MANIFEST.json to change with it?
> 
> You would at some point want the MANIFEST.json to change with it, that's true.
I was just thinking that it might be confusing if you got changes after just
running a test, since currently you can expect that running the tests won't
change anything.
> 
> I was thinking that only the importer would be responsible for making changes
to MANIFEST.json.
> 
> The main concern is that we don't want the test runner to do anything
unexpected or surprising to the user (modifying files), but we also want it to
behave the way people expect (finding new tests).

We talked in person and decided that it makes sense to update MANIFEST.json
during the test run, and print a message to the user if any new tests were
added.

[jeffcarp, 2017-01-23 22:27:51]

https://codereview.chromium.org/2644783003/diff/40001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py
(right):

https://codereview.chromium.org/2644783003/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py:750:
_log.warning('MANIFEST.json has been updated to reflect changes in
external/wpt!')
I spent an hour trying to write a unit test for this and couldn't quite get
there. Replacing Port._generate_manifest caused weird errors like this:

  Traceback (most recent call last):
    File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py",
line 316, in test_warn_if_new_test_found_in_manifest
      port.tests([])
    File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py",
line 670, in tests
      tests = self.real_tests(paths)
    File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py",
line 683, in real_tests
      skipped_directories, functools.partial(Port.is_test_file, self),
self.test_key)
    File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/common/find_files.py",
line 63, in find
      filesystem, base_dir, paths), skipped_directories, file_filter,
directory_sort_key)
    File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/common/find_files.py",
line 88, in _normalized_find
      for path in paths_to_walk))
  TypeError: type object argument after * must be a sequence, not generator

[qyearsley, 2017-01-24 01:56:03]

On 2017/01/23 at 22:27:51, jeffcarp wrote:
>
https://codereview.chromium.org/2644783003/diff/40001/third_party/WebKit/Tool...
> File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py
(right):
> 
>
https://codereview.chromium.org/2644783003/diff/40001/third_party/WebKit/Tool...
> third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py:750:
_log.warning('MANIFEST.json has been updated to reflect changes in
external/wpt!')
> I spent an hour trying to write a unit test for this and couldn't quite get
there. Replacing Port._generate_manifest caused weird errors like this:
> 
>   Traceback (most recent call last):
>     File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py",
line 316, in test_warn_if_new_test_found_in_manifest
>       port.tests([])
>     File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py",
line 670, in tests
>       tests = self.real_tests(paths)
>     File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py",
line 683, in real_tests
>       skipped_directories, functools.partial(Port.is_test_file, self),
self.test_key)
>     File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/common/find_files.py",
line 63, in find
>       filesystem, base_dir, paths), skipped_directories, file_filter,
directory_sort_key)
>     File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/common/find_files.py",
line 88, in _normalized_find
>       for path in paths_to_walk))
>   TypeError: type object argument after * must be a sequence, not generator

Hm, that sounds frustrating.

Could you update this CL with the latest patch so that I could reproduce that
error?

[jeffcarp, 2017-01-24 04:03:36]

On 2017/01/24 at 01:56:03, qyearsley wrote:
> On 2017/01/23 at 22:27:51, jeffcarp wrote:
> >
https://codereview.chromium.org/2644783003/diff/40001/third_party/WebKit/Tool...
> > File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py
(right):
> > 
> >
https://codereview.chromium.org/2644783003/diff/40001/third_party/WebKit/Tool...
> > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py:750:
_log.warning('MANIFEST.json has been updated to reflect changes in
external/wpt!')
> > I spent an hour trying to write a unit test for this and couldn't quite get
there. Replacing Port._generate_manifest caused weird errors like this:
> > 
> >   Traceback (most recent call last):
> >     File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py",
line 316, in test_warn_if_new_test_found_in_manifest
> >       port.tests([])
> >     File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py",
line 670, in tests
> >       tests = self.real_tests(paths)
> >     File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py",
line 683, in real_tests
> >       skipped_directories, functools.partial(Port.is_test_file, self),
self.test_key)
> >     File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/common/find_files.py",
line 63, in find
> >       filesystem, base_dir, paths), skipped_directories, file_filter,
directory_sort_key)
> >     File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/common/find_files.py",
line 88, in _normalized_find
> >       for path in paths_to_walk))
> >   TypeError: type object argument after * must be a sequence, not generator
> 
> Hm, that sounds frustrating.
> 
> Could you update this CL with the latest patch so that I could reproduce that
error?

I deleted the test since I couldn't finish it, I'll re-write it up to that state
and upload it.

[jeffcarp, 2017-01-24 20:32:39]

https://codereview.chromium.org/2644783003/diff/40001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py
(right):

https://codereview.chromium.org/2644783003/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py:750:
_log.warning('MANIFEST.json has been updated to reflect changes in
external/wpt!')
On 2017/01/23 at 22:27:51, jeffcarp wrote:
> I spent an hour trying to write a unit test for this and couldn't quite get
there. Replacing Port._generate_manifest caused weird errors like this:
> 
>   Traceback (most recent call last):
>     File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py",
line 316, in test_warn_if_new_test_found_in_manifest
>       port.tests([])
>     File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py",
line 670, in tests
>       tests = self.real_tests(paths)
>     File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py",
line 683, in real_tests
>       skipped_directories, functools.partial(Port.is_test_file, self),
self.test_key)
>     File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/common/find_files.py",
line 63, in find
>       filesystem, base_dir, paths), skipped_directories, file_filter,
directory_sort_key)
>     File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/common/find_files.py",
line 88, in _normalized_find
>       for path in paths_to_walk))
>   TypeError: type object argument after * must be a sequence, not generator

Replacing line 269 of base_unittest.py below with this is enough to cause the
error:
port = self.make_port(with_tests=True,
executive=MockExecutive(run_command_fn=lambda: ''))

I need to mock out the call that _generate_manifest makes so that the
MANIFEST.json changes mid-run, which is why I was originally trying to replace
run_command_fn but it led to this issue. Currently looking into it.

[jeffcarp, 2017-01-24 20:42:59]

https://codereview.chromium.org/2644783003/diff/40001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py
(right):

https://codereview.chromium.org/2644783003/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py:750:
_log.warning('MANIFEST.json has been updated to reflect changes in
external/wpt!')
On 2017/01/24 at 20:32:39, jeffcarp wrote:
> On 2017/01/23 at 22:27:51, jeffcarp wrote:
> > I spent an hour trying to write a unit test for this and couldn't quite get
there. Replacing Port._generate_manifest caused weird errors like this:
> > 
> >   Traceback (most recent call last):
> >     File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py",
line 316, in test_warn_if_new_test_found_in_manifest
> >       port.tests([])
> >     File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py",
line 670, in tests
> >       tests = self.real_tests(paths)
> >     File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py",
line 683, in real_tests
> >       skipped_directories, functools.partial(Port.is_test_file, self),
self.test_key)
> >     File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/common/find_files.py",
line 63, in find
> >       filesystem, base_dir, paths), skipped_directories, file_filter,
directory_sort_key)
> >     File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/common/find_files.py",
line 88, in _normalized_find
> >       for path in paths_to_walk))
> >   TypeError: type object argument after * must be a sequence, not generator
> 
> Replacing line 269 of base_unittest.py below with this is enough to cause the
error:
> port = self.make_port(with_tests=True,
executive=MockExecutive(run_command_fn=lambda: ''))
> 
> I need to mock out the call that _generate_manifest makes so that the
MANIFEST.json changes mid-run, which is why I was originally trying to replace
run_command_fn but it led to this issue. Currently looking into it.

I figured it out (see newest patch). My guess is that if run_command_fn returned
anything, it caused the above code to break in that way. Returning nothing seems
to solve the problem.

[jeffcarp, 2017-01-26 18:54:04]

On 2017/01/24 at 20:42:59, jeffcarp wrote:
>
https://codereview.chromium.org/2644783003/diff/40001/third_party/WebKit/Tool...
> File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py
(right):
> 
>
https://codereview.chromium.org/2644783003/diff/40001/third_party/WebKit/Tool...
> third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py:750:
_log.warning('MANIFEST.json has been updated to reflect changes in
external/wpt!')
> On 2017/01/24 at 20:32:39, jeffcarp wrote:
> > On 2017/01/23 at 22:27:51, jeffcarp wrote:
> > > I spent an hour trying to write a unit test for this and couldn't quite
get there. Replacing Port._generate_manifest caused weird errors like this:
> > > 
> > >   Traceback (most recent call last):
> > >     File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py",
line 316, in test_warn_if_new_test_found_in_manifest
> > >       port.tests([])
> > >     File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py",
line 670, in tests
> > >       tests = self.real_tests(paths)
> > >     File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py",
line 683, in real_tests
> > >       skipped_directories, functools.partial(Port.is_test_file, self),
self.test_key)
> > >     File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/common/find_files.py",
line 63, in find
> > >       filesystem, base_dir, paths), skipped_directories, file_filter,
directory_sort_key)
> > >     File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/common/find_files.py",
line 88, in _normalized_find
> > >       for path in paths_to_walk))
> > >   TypeError: type object argument after * must be a sequence, not
generator
> > 
> > Replacing line 269 of base_unittest.py below with this is enough to cause
the error:
> > port = self.make_port(with_tests=True,
executive=MockExecutive(run_command_fn=lambda: ''))
> > 
> > I need to mock out the call that _generate_manifest makes so that the
MANIFEST.json changes mid-run, which is why I was originally trying to replace
run_command_fn but it led to this issue. Currently looking into it.
> 
> I figured it out (see newest patch). My guess is that if run_command_fn
returned anything, it caused the above code to break in that way. Returning
nothing seems to solve the problem.

I think this should be ready to go if the approach is okay with you.

[qyearsley, 2017-01-26 19:00:44]

On 2017/01/26 at 18:54:04, jeffcarp wrote:
> On 2017/01/24 at 20:42:59, jeffcarp wrote:
> >
https://codereview.chromium.org/2644783003/diff/40001/third_party/WebKit/Tool...
> > File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py
(right):
> > 
> >
https://codereview.chromium.org/2644783003/diff/40001/third_party/WebKit/Tool...
> > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py:750:
_log.warning('MANIFEST.json has been updated to reflect changes in
external/wpt!')
> > On 2017/01/24 at 20:32:39, jeffcarp wrote:
> > > On 2017/01/23 at 22:27:51, jeffcarp wrote:
> > > > I spent an hour trying to write a unit test for this and couldn't quite
get there. Replacing Port._generate_manifest caused weird errors like this:
> > > > 
> > > >   Traceback (most recent call last):
> > > >     File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py",
line 316, in test_warn_if_new_test_found_in_manifest
> > > >       port.tests([])
> > > >     File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py",
line 670, in tests
> > > >       tests = self.real_tests(paths)
> > > >     File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py",
line 683, in real_tests
> > > >       skipped_directories, functools.partial(Port.is_test_file, self),
self.test_key)
> > > >     File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/common/find_files.py",
line 63, in find
> > > >       filesystem, base_dir, paths), skipped_directories, file_filter,
directory_sort_key)
> > > >     File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/common/find_files.py",
line 88, in _normalized_find
> > > >       for path in paths_to_walk))
> > > >   TypeError: type object argument after * must be a sequence, not
generator
> > > 
> > > Replacing line 269 of base_unittest.py below with this is enough to cause
the error:
> > > port = self.make_port(with_tests=True,
executive=MockExecutive(run_command_fn=lambda: ''))
> > > 
> > > I need to mock out the call that _generate_manifest makes so that the
MANIFEST.json changes mid-run, which is why I was originally trying to replace
run_command_fn but it led to this issue. Currently looking into it.
> > 
> > I figured it out (see newest patch). My guess is that if run_command_fn
returned anything, it caused the above code to break in that way. Returning
nothing seems to solve the problem.
> 
> I think this should be ready to go if the approach is okay with you.

LGTM :-)

[jeffcarp, 2017-01-26 20:25:11]

The CQ bit was checked by jeffcarp@chromium.org

[commit-bot: I haz the power, 2017-01-26 20:25:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 20:27:48]

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

[commit-bot: I haz the power, 2017-01-26 20:27:49]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[jeffcarp, 2017-01-26 22:26:39]

https://codereview.chromium.org/2644783003/diff/80001/third_party/WebKit/Tool...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py
(right):

https://codereview.chromium.org/2644783003/diff/80001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py:336:
port._generate_manifest = mock_generate_manifest  # pylint: disable=W0212
After rebasing I couldn't get the tests to pass without taking this different
approach - let me know if this works for you.

[qyearsley, 2017-01-26 23:08:52]

https://codereview.chromium.org/2644783003/diff/80001/third_party/WebKit/Tool...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py
(right):

https://codereview.chromium.org/2644783003/diff/80001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py:298:
# the result of running the update manifest command.
Since the contents are not important, it would be nice to make it
shorter/simpler, I think.

https://codereview.chromium.org/2644783003/diff/80001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py:336:
port._generate_manifest = mock_generate_manifest  # pylint: disable=W0212
On 2017/01/26 at 22:26:39, jeffcarp wrote:
> After rebasing I couldn't get the tests to pass without taking this different
approach - let me know if this works for you.

I think it's OK but might be nicer if it were simpler. If you were to change the
test to be simpler and test fewer things, then maybe a mock generate manifest
function wouldn't be necessary?

e.g. if we don't replace _generate_manifest, could we just assert that a
manifest command was called, without caring about whether it changes the
MANIFEST.json on the filesystem?

Also, # pylint: disable=W0212 can be changed to pylint: disable=protected-access

https://codereview.chromium.org/2644783003/diff/80001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py:338:
capture = OutputCapture()
Is it possible to use LoggingTestCase.assertLog (after making this test case
class a subclass of webkitpy.common.log_testing.LoggingTestCase), instead of
using OutputCapture?

[jeffcarp, 2017-01-27 23:03:56]

On 2017/01/26 at 23:08:52, qyearsley wrote:
>
https://codereview.chromium.org/2644783003/diff/80001/third_party/WebKit/Tool...
> File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py
(right):
> 
>
https://codereview.chromium.org/2644783003/diff/80001/third_party/WebKit/Tool...
>
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py:298:
# the result of running the update manifest command.
> Since the contents are not important, it would be nice to make it
shorter/simpler, I think.
> 
>
https://codereview.chromium.org/2644783003/diff/80001/third_party/WebKit/Tool...
>
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py:336:
port._generate_manifest = mock_generate_manifest  # pylint: disable=W0212
> On 2017/01/26 at 22:26:39, jeffcarp wrote:
> > After rebasing I couldn't get the tests to pass without taking this
different approach - let me know if this works for you.
> 
> I think it's OK but might be nicer if it were simpler. If you were to change
the test to be simpler and test fewer things, then maybe a mock generate
manifest function wouldn't be necessary?
> 
> e.g. if we don't replace _generate_manifest, could we just assert that a
manifest command was called, without caring about whether it changes the
MANIFEST.json on the filesystem?
> 
> Also, # pylint: disable=W0212 can be changed to pylint:
disable=protected-access
> 
>
https://codereview.chromium.org/2644783003/diff/80001/third_party/WebKit/Tool...
>
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py:338:
capture = OutputCapture()
> Is it possible to use LoggingTestCase.assertLog (after making this test case
class a subclass of webkitpy.common.log_testing.LoggingTestCase), instead of
using OutputCapture?

Uploaded a new patch based on our discussion.

[qyearsley, 2017-01-27 23:24:49]

https://codereview.chromium.org/2644783003/diff/100001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py
(right):

https://codereview.chromium.org/2644783003/diff/100001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py:769: 'with
your changes. See https://crbug.com/666957.'))
Can you call log methods with a tuple like this? I think that this might result
in just logging a stringified tuple. In most places we print long messages with
either:

        _log.info('Generating MANIFEST.json, this might result in changes to
MANIFEST.json\n'
                  'in your working directory. Please commit them along with your
changes.\n'
                  'See https://crbug.com/666957.')

or:
        _log.info('Generating MANIFEST.json, this might result in changes to
MANIFEST.json')
        _log.info('in your working directory. Please commit them along with your
changes.')
        _log.info('See https://crbug.com/666957.')

Anyway, I think this message is good for now but seems like a temporary thing.
I wonder what the ideal behavior should be? I'm imagining maybe the ideal
behavior might
be still be staging the change for you if necessary and only printing a message
when
a change was made?

[jeffcarp, 2017-01-27 23:50:41]

On 2017/01/27 at 23:24:49, qyearsley wrote:
>
https://codereview.chromium.org/2644783003/diff/100001/third_party/WebKit/Too...
> File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py
(right):
> 
>
https://codereview.chromium.org/2644783003/diff/100001/third_party/WebKit/Too...
> third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py:769: 'with
your changes. See https://crbug.com/666957.'))
> Can you call log methods with a tuple like this? I think that this might
result in just logging a stringified tuple. In most places we print long
messages with either:
> 
>         _log.info('Generating MANIFEST.json, this might result in changes to
MANIFEST.json\n'
>                   'in your working directory. Please commit them along with
your changes.\n'
>                   'See https://crbug.com/666957.')
> 
> or:
>         _log.info('Generating MANIFEST.json, this might result in changes to
MANIFEST.json')
>         _log.info('in your working directory. Please commit them along with
your changes.')
>         _log.info('See https://crbug.com/666957.')
> 
> Anyway, I think this message is good for now but seems like a temporary thing.
> I wonder what the ideal behavior should be? I'm imagining maybe the ideal
behavior might
> be still be staging the change for you if necessary and only printing a
message when
> a change was made?

Oops, I think I just got my Python idioms mixed up, thanks.

Only printing a message if a change was made would be preferable, yes. I can
make a bug.

[jeffcarp, 2017-01-27 23:50:44]

The CQ bit was checked by jeffcarp@chromium.org

[jeffcarp, 2017-01-27 23:50:45]

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/2644783003/#ps120001
(title: "Fix multiline tuple->string")

[commit-bot: I haz the power, 2017-01-27 23:51:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-28 00:38:14]

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

[commit-bot: I haz the power, 2017-01-28 00:38:15]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[jeffcarp, 2017-01-30 03:41:34]

On 2017/01/28 at 00:38:15, commit-bot wrote:
> Try jobs failed on following builders:
>   win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

Possibly flaky faliures on win and linux headles rel_ng? Going to try again.

[jeffcarp, 2017-01-30 03:41:38]

The CQ bit was checked by jeffcarp@chromium.org

[commit-bot: I haz the power, 2017-01-30 03:41:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-30 04:25:23]

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

[commit-bot: I haz the power, 2017-01-30 04:25:25]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[qyearsley, 2017-02-01 17:58:32]

On 2017/01/30 at 04:25:25, commit-bot wrote:
> Try jobs failed on following builders:
>   win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

Looks like this might be a real consistent error rather than a flaky error:
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel...

Specific log:
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel...

Running Tools/Scripts/lint-test-expectations actually invokes a code-path that
ends up generating the manifest file -- maybe we don't want this to happen?
Although it should be possible to do.

Running ['E:\\b\\depot_tools\\python276_bin\\python.exe',
'E:\\b\\c\\b\\win\\src\\third_party\\WebKit\\Tools\\Scripts\\lint-test-expectations',
'--json', 'c:\\users\\chrome~2\\appdata\\local\\temp\\tmpklsp07'] in None (env:
None)

Specifically, when reading the test expectations file, it checks whether tests
are reference tests in order to add a warning about Rebaseline/NeedsRebaseline
expectations for reference tests, since reference tests can't be rebaselined.

The simplest solution would be to stop adding this warning, but I wonder what
where that "%1" comes from?

[qyearsley, 2017-02-01 18:23:39]

On 2017/02/01 at 17:58:32, qyearsley wrote:
> On 2017/01/30 at 04:25:25, commit-bot wrote:
> > Try jobs failed on following builders:
> >   win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
> 
> Looks like this might be a real consistent error rather than a flaky error:
>
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel...
> 
> Specific log:
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel...
> 
> Running Tools/Scripts/lint-test-expectations actually invokes a code-path that
ends up generating the manifest file -- maybe we don't want this to happen?
Although it should be possible to do.
> 
> Running ['E:\\b\\depot_tools\\python276_bin\\python.exe',
'E:\\b\\c\\b\\win\\src\\third_party\\WebKit\\Tools\\Scripts\\lint-test-expectations',
'--json', 'c:\\users\\chrome~2\\appdata\\local\\temp\\tmpklsp07'] in None (env:
None)
> 
> Specifically, when reading the test expectations file, it checks whether tests
are reference tests in order to add a warning about Rebaseline/NeedsRebaseline
expectations for reference tests, since reference tests can't be rebaselined.
> 
> The simplest solution would be to stop adding this warning, but I wonder what
where that "%1" comes from?

Note, running lint-test-expectations on any platform will end up updating the
manifest, which is not what I would have expected. Do you think that it's OK to
remove the warning about rebaseline expectations for ref tests at
https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...

[jeffcarp, 2017-02-10 02:30:26]

On 2017/02/01 at 18:23:39, qyearsley wrote:
> On 2017/02/01 at 17:58:32, qyearsley wrote:
> > On 2017/01/30 at 04:25:25, commit-bot wrote:
> > > Try jobs failed on following builders:
> > >   win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
> > 
> > Looks like this might be a real consistent error rather than a flaky error:
> >
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel...
> > 
> > Specific log:
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel...
> > 
> > Running Tools/Scripts/lint-test-expectations actually invokes a code-path
that ends up generating the manifest file -- maybe we don't want this to happen?
Although it should be possible to do.
> > 
> > Running ['E:\\b\\depot_tools\\python276_bin\\python.exe',
'E:\\b\\c\\b\\win\\src\\third_party\\WebKit\\Tools\\Scripts\\lint-test-expectations',
'--json', 'c:\\users\\chrome~2\\appdata\\local\\temp\\tmpklsp07'] in None (env:
None)
> > 
> > Specifically, when reading the test expectations file, it checks whether
tests are reference tests in order to add a warning about
Rebaseline/NeedsRebaseline expectations for reference tests, since reference
tests can't be rebaselined.
> > 
> > The simplest solution would be to stop adding this warning, but I wonder
what where that "%1" comes from?
> 
> Note, running lint-test-expectations on any platform will end up updating the
manifest, which is not what I would have expected. Do you think that it's OK to
remove the warning about rebaseline expectations for ref tests at
https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...

I'm not sure I understand how those two problems are related?

[qyearsley, 2017-02-10 17:29:49]

On 2017/02/10 at 02:30:26, jeffcarp wrote:
> On 2017/02/01 at 18:23:39, qyearsley wrote:
> > On 2017/02/01 at 17:58:32, qyearsley wrote:
> > > On 2017/01/30 at 04:25:25, commit-bot wrote:
> > > > Try jobs failed on following builders:
> > > >   win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
> > > 
> > > Looks like this might be a real consistent error rather than a flaky
error:
> > >
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel...
> > > 
> > > Specific log:
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel...
> > > 
> > > Running Tools/Scripts/lint-test-expectations actually invokes a code-path
that ends up generating the manifest file -- maybe we don't want this to happen?
Although it should be possible to do.
> > > 
> > > Running ['E:\\b\\depot_tools\\python276_bin\\python.exe',
'E:\\b\\c\\b\\win\\src\\third_party\\WebKit\\Tools\\Scripts\\lint-test-expectations',
'--json', 'c:\\users\\chrome~2\\appdata\\local\\temp\\tmpklsp07'] in None (env:
None)
> > > 
> > > Specifically, when reading the test expectations file, it checks whether
tests are reference tests in order to add a warning about
Rebaseline/NeedsRebaseline expectations for reference tests, since reference
tests can't be rebaselined.
> > > 
> > > The simplest solution would be to stop adding this warning, but I wonder
what where that "%1" comes from?
> > 
> > Note, running lint-test-expectations on any platform will end up updating
the manifest, which is not what I would have expected. Do you think that it's OK
to remove the warning about rebaseline expectations for ref tests at
https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...
> 
> I'm not sure I understand how those two problems are related?

You're right, these two problems are not related. The two separate issues are:

 1. [Bug] On Windows, generating the manifest when reading the expectations
results in a mysterious error involving "%1"
 2. [Unexpected behavior] Linting the test expectations ends up reading the
manifest, and thus generating the manifest.

[jeffcarp, 2017-02-10 18:17:10]

On 2017/02/10 at 17:29:49, qyearsley wrote:
> On 2017/02/10 at 02:30:26, jeffcarp wrote:
> > On 2017/02/01 at 18:23:39, qyearsley wrote:
> > > On 2017/02/01 at 17:58:32, qyearsley wrote:
> > > > On 2017/01/30 at 04:25:25, commit-bot wrote:
> > > > > Try jobs failed on following builders:
> > > > >   win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
> > > > 
> > > > Looks like this might be a real consistent error rather than a flaky
error:
> > > >
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel...
> > > > 
> > > > Specific log:
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel...
> > > > 
> > > > Running Tools/Scripts/lint-test-expectations actually invokes a
code-path that ends up generating the manifest file -- maybe we don't want this
to happen? Although it should be possible to do.
> > > > 
> > > > Running ['E:\\b\\depot_tools\\python276_bin\\python.exe',
'E:\\b\\c\\b\\win\\src\\third_party\\WebKit\\Tools\\Scripts\\lint-test-expectations',
'--json', 'c:\\users\\chrome~2\\appdata\\local\\temp\\tmpklsp07'] in None (env:
None)
> > > > 
> > > > Specifically, when reading the test expectations file, it checks whether
tests are reference tests in order to add a warning about
Rebaseline/NeedsRebaseline expectations for reference tests, since reference
tests can't be rebaselined.
> > > > 
> > > > The simplest solution would be to stop adding this warning, but I wonder
what where that "%1" comes from?
> > > 
> > > Note, running lint-test-expectations on any platform will end up updating
the manifest, which is not what I would have expected. Do you think that it's OK
to remove the warning about rebaseline expectations for ref tests at
https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...
> > 
> > I'm not sure I understand how those two problems are related?
> 
> You're right, these two problems are not related. The two separate issues are:
> 
>  1. [Bug] On Windows, generating the manifest when reading the expectations
results in a mysterious error involving "%1"
>  2. [Unexpected behavior] Linting the test expectations ends up reading the
manifest, and thus generating the manifest.

I'm sorry I'm not sure what I'm missing but where are you seeing the %1? I don't
see it anywhere.

[jeffcarp, 2017-02-10 18:19:03]

On 2017/02/10 at 18:17:10, jeffcarp wrote:
> On 2017/02/10 at 17:29:49, qyearsley wrote:
> > On 2017/02/10 at 02:30:26, jeffcarp wrote:
> > > On 2017/02/01 at 18:23:39, qyearsley wrote:
> > > > On 2017/02/01 at 17:58:32, qyearsley wrote:
> > > > > On 2017/01/30 at 04:25:25, commit-bot wrote:
> > > > > > Try jobs failed on following builders:
> > > > > >   win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
> > > > > 
> > > > > Looks like this might be a real consistent error rather than a flaky
error:
> > > > >
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel...
> > > > > 
> > > > > Specific log:
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel...
> > > > > 
> > > > > Running Tools/Scripts/lint-test-expectations actually invokes a
code-path that ends up generating the manifest file -- maybe we don't want this
to happen? Although it should be possible to do.
> > > > > 
> > > > > Running ['E:\\b\\depot_tools\\python276_bin\\python.exe',
'E:\\b\\c\\b\\win\\src\\third_party\\WebKit\\Tools\\Scripts\\lint-test-expectations',
'--json', 'c:\\users\\chrome~2\\appdata\\local\\temp\\tmpklsp07'] in None (env:
None)
> > > > > 
> > > > > Specifically, when reading the test expectations file, it checks
whether tests are reference tests in order to add a warning about
Rebaseline/NeedsRebaseline expectations for reference tests, since reference
tests can't be rebaselined.
> > > > > 
> > > > > The simplest solution would be to stop adding this warning, but I
wonder what where that "%1" comes from?
> > > > 
> > > > Note, running lint-test-expectations on any platform will end up
updating the manifest, which is not what I would have expected. Do you think
that it's OK to remove the warning about rebaseline expectations for ref tests
at
https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...
> > > 
> > > I'm not sure I understand how those two problems are related?
> > 
> > You're right, these two problems are not related. The two separate issues
are:
> > 
> >  1. [Bug] On Windows, generating the manifest when reading the expectations
results in a mysterious error involving "%1"
> >  2. [Unexpected behavior] Linting the test expectations ends up reading the
manifest, and thus generating the manifest.
> 
> I'm sorry I'm not sure what I'm missing but where are you seeing the %1? I
don't see it anywhere.

I'm not sure how to go about diagnosing this issue aside from getting a Windows
machine and running the manifest script on it.

[jeffcarp, 2017-02-12 22:39:16]

On 2017/02/10 at 18:19:03, jeffcarp wrote:
> On 2017/02/10 at 18:17:10, jeffcarp wrote:
> > On 2017/02/10 at 17:29:49, qyearsley wrote:
> > > On 2017/02/10 at 02:30:26, jeffcarp wrote:
> > > > On 2017/02/01 at 18:23:39, qyearsley wrote:
> > > > > On 2017/02/01 at 17:58:32, qyearsley wrote:
> > > > > > On 2017/01/30 at 04:25:25, commit-bot wrote:
> > > > > > > Try jobs failed on following builders:
> > > > > > >   win_chromium_rel_ng on master.tryserver.chromium.win
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
> > > > > > 
> > > > > > Looks like this might be a real consistent error rather than a flaky
error:
> > > > > >
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel...
> > > > > > 
> > > > > > Specific log:
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel...
> > > > > > 
> > > > > > Running Tools/Scripts/lint-test-expectations actually invokes a
code-path that ends up generating the manifest file -- maybe we don't want this
to happen? Although it should be possible to do.
> > > > > > 
> > > > > > Running ['E:\\b\\depot_tools\\python276_bin\\python.exe',
'E:\\b\\c\\b\\win\\src\\third_party\\WebKit\\Tools\\Scripts\\lint-test-expectations',
'--json', 'c:\\users\\chrome~2\\appdata\\local\\temp\\tmpklsp07'] in None (env:
None)
> > > > > > 
> > > > > > Specifically, when reading the test expectations file, it checks
whether tests are reference tests in order to add a warning about
Rebaseline/NeedsRebaseline expectations for reference tests, since reference
tests can't be rebaselined.
> > > > > > 
> > > > > > The simplest solution would be to stop adding this warning, but I
wonder what where that "%1" comes from?
> > > > > 
> > > > > Note, running lint-test-expectations on any platform will end up
updating the manifest, which is not what I would have expected. Do you think
that it's OK to remove the warning about rebaseline expectations for ref tests
at
https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...
> > > > 
> > > > I'm not sure I understand how those two problems are related?
> > > 
> > > You're right, these two problems are not related. The two separate issues
are:
> > > 
> > >  1. [Bug] On Windows, generating the manifest when reading the
expectations results in a mysterious error involving "%1"
> > >  2. [Unexpected behavior] Linting the test expectations ends up reading
the manifest, and thus generating the manifest.
> > 
> > I'm sorry I'm not sure what I'm missing but where are you seeing the %1? I
don't see it anywhere.
> 
> I'm not sure how to go about diagnosing this issue aside from getting a
Windows machine and running the manifest script on it.

From our discussion Friday: we think the issue with the initial approach was
that the MANIFEST.json regeneration was happening after sharding, so bugs were
happening due to many processes regenerating and writing the file at once.

I moved the regeneration up to before sharding happens. The trybots are passing
now. I still need to write unit tests but lmk what you think.

[foolip, 2017-02-16 07:20:25]

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

[foolip, 2017-02-16 07:20:26]

Now just missing a test?

If you remove MANIFEST.json, this is the LayoutTests/external/.gitignore I
tried:

# MANIFEST.json is generated in the tree but shouldn't be committed.
/wpt/MANIFEST.json

https://codereview.chromium.org/2644783003/diff/220001/third_party/WebKit/Too...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py
(right):

https://codereview.chromium.org/2644783003/diff/220001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:565:
_log.info('Generating MANIFEST.json, this might result in changes to
MANIFEST.json\n'
I have verified that removing MANIFEST.json from the repo entirely, this still
works. It's slow the first time, but quite fast the second time.

In https://bugs.chromium.org/p/chromium/issues/detail?id=666957#c24 I suggested
we just delete MANIFEST.json and then try to optimize the first run. The message
here could be changed to "Generating MANIFEST.json, this can be slow.
https://crbug.com/666957" and then removed entirely when it's no longer slow.

https://codereview.chromium.org/2644783003/diff/220001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py
(right):

https://codereview.chromium.org/2644783003/diff/220001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py:747: def
_manifest_items_for_path(self, path_in_wpt):
Looks like this is unused now?

[jeffcarp, 2017-02-21 19:32:11]

@qyearsley @foolip I just updated this CL with a new in-progress version we
talked about.

https://codereview.chromium.org/2644783003/diff/240001/third_party/WebKit/Too...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py
(right):

https://codereview.chromium.org/2644783003/diff/240001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:105:
# but to know if we need to generate MANIFEST.json we need to find tests...
I need the most help here figuring out if there's any way we can tell whether we
need to regenerate MANIFEST.json or not. To me it seems like we'll always have
to regenerate it.

[jeffcarp, 2017-02-21 19:33:16]

https://codereview.chromium.org/2644783003/diff/240001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py
(right):

https://codereview.chromium.org/2644783003/diff/240001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py:772: return
None
This isn't part of my CL, not sure how it got in here.

[qyearsley, 2017-02-21 22:47:57]

https://codereview.chromium.org/2644783003/diff/240001/third_party/WebKit/Too...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py
(right):

https://codereview.chromium.org/2644783003/diff/240001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:105:
# but to know if we need to generate MANIFEST.json we need to find tests...
On 2017/02/21 at 19:32:11, jeffcarp wrote:
> I need the most help here figuring out if there's any way we can tell whether
we need to regenerate MANIFEST.json or not. To me it seems like we'll always
have to regenerate it.

Always regenerating it seems like a reasonable option to me :-)

https://codereview.chromium.org/2644783003/diff/240001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py
(right):

https://codereview.chromium.org/2644783003/diff/240001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py:772: return
None
On 2017/02/21 at 19:33:16, jeffcarp wrote:
> This isn't part of my CL, not sure how it got in here.

This part was changed in https://codereview.chromium.org/2681733005 (after the
first patchset of this CL was uploaded), so it could be a merge issue

[foolip, 2017-02-23 02:51:30]

WPT_BASE_MANIFEST.json is too big to show, does it now include itself? And if it
doesn't what are the steps to update it in such a way that it still doesn't
include itself?

https://codereview.chromium.org/2644783003/diff/240001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/external/.gitignore (right):

https://codereview.chromium.org/2644783003/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/.gitignore:1: wpt/MANIFEST.json
Have you checked manually that this works? It probably does, but it would also
ignore a LayoutTests/external/wpt/foo/bar/wpt/MANIFEST.json, and adding a
leading / would fix that.

https://codereview.chromium.org/2644783003/diff/240001/third_party/WebKit/Too...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py
(right):

https://codereview.chromium.org/2644783003/diff/240001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:105:
# but to know if we need to generate MANIFEST.json we need to find tests...
On 2017/02/21 22:47:57, qyearsley wrote:
> On 2017/02/21 at 19:32:11, jeffcarp wrote:
> > I need the most help here figuring out if there's any way we can tell
whether
> we need to regenerate MANIFEST.json or not. To me it seems like we'll always
> have to regenerate it.
> 
> Always regenerating it seems like a reasonable option to me :-)

On some systems I think the mtime of LayoutTests/external/wpt could be compared
with the mtime of MANIFEST.json, but I wouldn't assume that's reliable. As long
as it's pretty fast, always regenerating SGTM.

[jeffcarp, 2017-02-28 02:44:10]

This CL is now ready for review after updating based on what we talked about.

The only problem I ran into was updating my MANIFEST.json for the first time.
Deleting MANIFEST.json and starting over fixed the problem. @qyearsley can you
try out this patch and run a few layout tests on your local machine? Here's the
error I was getting (it was from the manifest script itself):

jeffcarp0:~/chromium/src/third_party/WebKit Jeff
Carpenter@regenerate-manifest-on-wpt-test-invokation % python
/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/thirdparty/wpt/wpt/manifest
--work --tests-root
/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/LayoutTests/external/wpt
DEBUG:manifest:Opening manifest at
/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/LayoutTests/external/wpt/MANIFEST.json
INFO:manifest:Updating manifest
Traceback (most recent call last):
  File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/thirdparty/wpt/wpt/manifest",
line 8, in <module>
    os.path.abspath(os.path.dirname(__file__)))
  File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/thirdparty/wpt/wpt/tools/manifest/update.py",
line 104, in main
    update_from_cli(**vars(opts))
  File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/thirdparty/wpt/wpt/tools/manifest/update.py",
line 46, in update_from_cli
    working_copy=kwargs["work"])
  File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/thirdparty/wpt/wpt/tools/manifest/update.py",
line 21, in update
    return manifest.update(tree)
  File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/thirdparty/wpt/wpt/tools/manifest/manifest.py",
line 81, in update
    old_files[old_type].remove(rel_path)
KeyError: './MANIFEST.json'

[jeffcarp, 2017-02-28 02:44:57]

On 2017/02/28 at 02:44:10, jeffcarp wrote:
> This CL is now ready for review after updating based on what we talked about.
> 
> The only problem I ran into was updating my MANIFEST.json for the first time.
Deleting MANIFEST.json and starting over fixed the problem. @qyearsley can you
try out this patch and run a few layout tests on your local machine? Here's the
error I was getting (it was from the manifest script itself):
> 
> jeffcarp0:~/chromium/src/third_party/WebKit Jeff
Carpenter@regenerate-manifest-on-wpt-test-invokation % python
/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/thirdparty/wpt/wpt/manifest
--work --tests-root
/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/LayoutTests/external/wpt
> DEBUG:manifest:Opening manifest at
/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/LayoutTests/external/wpt/MANIFEST.json
> INFO:manifest:Updating manifest
> Traceback (most recent call last):
>   File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/thirdparty/wpt/wpt/manifest",
line 8, in <module>
>     os.path.abspath(os.path.dirname(__file__)))
>   File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/thirdparty/wpt/wpt/tools/manifest/update.py",
line 104, in main
>     update_from_cli(**vars(opts))
>   File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/thirdparty/wpt/wpt/tools/manifest/update.py",
line 46, in update_from_cli
>     working_copy=kwargs["work"])
>   File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/thirdparty/wpt/wpt/tools/manifest/update.py",
line 21, in update
>     return manifest.update(tree)
>   File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/thirdparty/wpt/wpt/tools/manifest/manifest.py",
line 81, in update
>     old_files[old_type].remove(rel_path)
> KeyError: './MANIFEST.json'

Also, is MANIFEST.json too big to be uploaded to Rietveld in its entirety?
Trying to figure out why WPT_BASE_MANIFEST.json is not showing up.

[jeffcarp, 2017-02-28 02:51:00]

On 2017/02/28 at 02:44:57, jeffcarp wrote:
> On 2017/02/28 at 02:44:10, jeffcarp wrote:
> > This CL is now ready for review after updating based on what we talked
about.
> > 
> > The only problem I ran into was updating my MANIFEST.json for the first
time. Deleting MANIFEST.json and starting over fixed the problem. @qyearsley can
you try out this patch and run a few layout tests on your local machine? Here's
the error I was getting (it was from the manifest script itself):
> > 
> > jeffcarp0:~/chromium/src/third_party/WebKit Jeff
Carpenter@regenerate-manifest-on-wpt-test-invokation % python
/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/thirdparty/wpt/wpt/manifest
--work --tests-root
/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/LayoutTests/external/wpt
> > DEBUG:manifest:Opening manifest at
/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/LayoutTests/external/wpt/MANIFEST.json
> > INFO:manifest:Updating manifest
> > Traceback (most recent call last):
> >   File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/thirdparty/wpt/wpt/manifest",
line 8, in <module>
> >     os.path.abspath(os.path.dirname(__file__)))
> >   File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/thirdparty/wpt/wpt/tools/manifest/update.py",
line 104, in main
> >     update_from_cli(**vars(opts))
> >   File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/thirdparty/wpt/wpt/tools/manifest/update.py",
line 46, in update_from_cli
> >     working_copy=kwargs["work"])
> >   File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/thirdparty/wpt/wpt/tools/manifest/update.py",
line 21, in update
> >     return manifest.update(tree)
> >   File
"/usr/local/google/home/jeffcarp/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/thirdparty/wpt/wpt/tools/manifest/manifest.py",
line 81, in update
> >     old_files[old_type].remove(rel_path)
> > KeyError: './MANIFEST.json'
> 
> Also, is MANIFEST.json too big to be uploaded to Rietveld in its entirety?
Trying to figure out why WPT_BASE_MANIFEST.json is not showing up.

When I remove the entry for MANIFEST.json from MANIFEST.json, the script works
fine, so I'll investigate why that causes the script to break.

[foolip, 2017-02-28 04:49:17]

This all LGTM except where not understood, leaving proper and final review to
qyearsley@.

Improving things by moving to out/ directory would be best left as a follow-up
if it would delay this fix by even a single day :)

https://codereview.chromium.org/2644783003/diff/240001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/external/.gitignore (right):

https://codereview.chromium.org/2644783003/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/.gitignore:1: wpt/MANIFEST.json
On 2017/02/23 02:51:30, foolip wrote:
> Have you checked manually that this works? It probably does, but it would also
> ignore a LayoutTests/external/wpt/foo/bar/wpt/MANIFEST.json, and adding a
> leading / would fix that.

Ping.

https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/common/host_mock.py (right):

https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/common/host_mock.py:86: 'LayoutTests',
'external', 'WPT_BASE_MANIFEST.json')
Had an idea while looking at this line. Would it be possible to put the
generated MANIFEST.json in the same dir next to this to trivially get around it
including itself? Or even better, somewhere in the out/ directory?

Presumably the code that knows where MANIFEST.json is expected to be is under
our control so we could just change that? (If there's some upstream code that
uses the location of MANIFEST.json, that'd be a problem, but we could change
that too probably if it's the right thing.)

https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py
(right):

https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py:190:
self.assertEqual(port.host.executive.calls, [
I don't understand this assert, leaving it to qyearsley@ to understand :)

[qyearsley, 2017-02-28 18:54:45]

https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/common/host_mock.py (right):

https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/common/host_mock.py:86: 'LayoutTests',
'external', 'WPT_BASE_MANIFEST.json')
On 2017/02/28 at 04:49:17, foolip wrote:
> Had an idea while looking at this line. Would it be possible to put the
generated MANIFEST.json in the same dir next to this to trivially get around it
including itself? Or even better, somewhere in the out/ directory?
>
> Presumably the code that knows where MANIFEST.json is expected to be is under
our control so we could just change that? (If there's some upstream code that
uses the location of MANIFEST.json, that'd be a problem, but we could change
that too probably if it's the right thing.)

This sounds reasonable to me -- I'd be fine with this change in this CL or a
follow-up CL if you think it's a good idea. The manifest generator script takes
a --path argument which should be the path to the manifest file. The
_ensure_manifest method would still copy the base over to the manifest path and
then run the manifest generator command.

If this is done, the knowledge about where the manifest file is should be kept
in one place, probably webkitpy/w3c/wpt_manifest.py.

https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py
(right):

https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:102:
# Regenerate MANIFEST.json from template, necessary for WPT metadata
Nit: period at end of comment. Could also spell out web-platform-tests,
optionally.

https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:556:
def _ensure_manifest(self):
Do you think it makes sense to make this a public static method or top-level
function in webkitpy/w3c/wpt_manifest.py?

https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:558:
'LayoutTests', 'external', 'wpt', 'MANIFEST.json')
Optional idea: These paths relative to webkit base could potentially be
constants in webkitpy/w3c/wpt_manifest.py.

https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:560:
'LayoutTests', 'external', 'WPT_BASE_MANIFEST.json')
Here we're assuming that WPT_BASE_MANIFEST.json always exists.
This is fine, although you could also make this explicit:

    assert self._filesystem.exists(manifest_base_path)

Or, you could even make it just log an error and generate the whole manifest
from scratch anyway.

https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py
(right):

https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py:181:

As a convention, when explicitly testing protected methods in unit tests, I've
been adding a comment above the test methods like:

    # Tests for protected methods - pylint: disable=protected-access

To avoid having to repeat the pylint disable comment below. Does this sound like
an OK idea? I think that ideally one should test public methods first and not
necessarily test protected/private methods, but often having tests for private
methods is better than no tests, and in this case I think explicitly saying this
with a pylint disable comment is fine.

https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py:184:
port = host.port_factory.get('test-mac-mac10.10')
If the test is not asserting anything that's port-specific, you can also do
`port = host.port_factory.get()`, which for MockHost I think should return a
TestPort of some kind by default.

https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py:190:
self.assertEqual(port.host.executive.calls, [
On 2017/02/28 at 04:49:17, foolip wrote:
> I don't understand this assert, leaving it to qyearsley@ to understand :)

This is asserting that one subprocess was invoked, which is the manifest
generator script; this seems like what we want :-)

https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py:193:
'--work', '--tests-root',
'/mock-checkout/third_party/WebKit/LayoutTests/external/wpt']])
Nit: Optionally, you could also format this as something like:

self.assertEqual(
    port.host.executive.calls,
    [
        [
            'python',
            webkit_base + '/Tools/Scripts/webkitpy/thirdparty/wpt/wpt/manifest',
            '--work',
            '--tests-root',
            webkit_base + '/LayoutTests/external/wpt',
        ]
    ])

[jeffcarp, 2017-02-28 21:18:46]

On 2017/02/28 at 18:54:45, qyearsley wrote:
>
https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
> File third_party/WebKit/Tools/Scripts/webkitpy/common/host_mock.py (right):
> 
>
https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
> third_party/WebKit/Tools/Scripts/webkitpy/common/host_mock.py:86:
'LayoutTests', 'external', 'WPT_BASE_MANIFEST.json')
> On 2017/02/28 at 04:49:17, foolip wrote:
> > Had an idea while looking at this line. Would it be possible to put the
generated MANIFEST.json in the same dir next to this to trivially get around it
including itself? Or even better, somewhere in the out/ directory?
> >
> > Presumably the code that knows where MANIFEST.json is expected to be is
under our control so we could just change that? (If there's some upstream code
that uses the location of MANIFEST.json, that'd be a problem, but we could
change that too probably if it's the right thing.)
> 
> This sounds reasonable to me -- I'd be fine with this change in this CL or a
follow-up CL if you think it's a good idea. The manifest generator script takes
a --path argument which should be the path to the manifest file. The
_ensure_manifest method would still copy the base over to the manifest path and
then run the manifest generator command.
> 
> If this is done, the knowledge about where the manifest file is should be kept
in one place, probably webkitpy/w3c/wpt_manifest.py.
> 
>
https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
> File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py
(right):
> 
>
https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
>
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:102:
# Regenerate MANIFEST.json from template, necessary for WPT metadata
> Nit: period at end of comment. Could also spell out web-platform-tests,
optionally.
> 
>
https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
>
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:556:
def _ensure_manifest(self):
> Do you think it makes sense to make this a public static method or top-level
function in webkitpy/w3c/wpt_manifest.py?
> 
>
https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
>
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:558:
'LayoutTests', 'external', 'wpt', 'MANIFEST.json')
> Optional idea: These paths relative to webkit base could potentially be
constants in webkitpy/w3c/wpt_manifest.py.
> 
>
https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
>
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:560:
'LayoutTests', 'external', 'WPT_BASE_MANIFEST.json')
> Here we're assuming that WPT_BASE_MANIFEST.json always exists.
> This is fine, although you could also make this explicit:
> 
>     assert self._filesystem.exists(manifest_base_path)
> 
> Or, you could even make it just log an error and generate the whole manifest
from scratch anyway.
> 
>
https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
> File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py
(right):
> 
>
https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
>
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py:181:

> As a convention, when explicitly testing protected methods in unit tests, I've
been adding a comment above the test methods like:
> 
>     # Tests for protected methods - pylint: disable=protected-access
> 
> To avoid having to repeat the pylint disable comment below. Does this sound
like an OK idea? I think that ideally one should test public methods first and
not necessarily test protected/private methods, but often having tests for
private methods is better than no tests, and in this case I think explicitly
saying this with a pylint disable comment is fine.
> 
>
https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
>
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py:184:
port = host.port_factory.get('test-mac-mac10.10')
> If the test is not asserting anything that's port-specific, you can also do
`port = host.port_factory.get()`, which for MockHost I think should return a
TestPort of some kind by default.
> 
>
https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
>
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py:190:
self.assertEqual(port.host.executive.calls, [
> On 2017/02/28 at 04:49:17, foolip wrote:
> > I don't understand this assert, leaving it to qyearsley@ to understand :)
> 
> This is asserting that one subprocess was invoked, which is the manifest
generator script; this seems like what we want :-)
> 
>
https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
>
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py:193:
'--work', '--tests-root',
'/mock-checkout/third_party/WebKit/LayoutTests/external/wpt']])
> Nit: Optionally, you could also format this as something like:
> 
> self.assertEqual(
>     port.host.executive.calls,
>     [
>         [
>             'python',
>             webkit_base +
'/Tools/Scripts/webkitpy/thirdparty/wpt/wpt/manifest',
>             '--work',
>             '--tests-root',
>             webkit_base + '/LayoutTests/external/wpt',
>         ]
>     ])

Question before I send my individual feedback responses: it looks like
WPT_BASE_MANIFEST.json is too big for Rietveld (it's showing -0 +0 lines changed
and no diff). Will it be included in this commit or should I move this code
review to Gerrit?

[jeffcarp, 2017-02-28 21:46:06]

On 2017/02/28 at 21:18:46, jeffcarp wrote:
> On 2017/02/28 at 18:54:45, qyearsley wrote:
> >
https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
> > File third_party/WebKit/Tools/Scripts/webkitpy/common/host_mock.py (right):
> > 
> >
https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
> > third_party/WebKit/Tools/Scripts/webkitpy/common/host_mock.py:86:
'LayoutTests', 'external', 'WPT_BASE_MANIFEST.json')
> > On 2017/02/28 at 04:49:17, foolip wrote:
> > > Had an idea while looking at this line. Would it be possible to put the
generated MANIFEST.json in the same dir next to this to trivially get around it
including itself? Or even better, somewhere in the out/ directory?
> > >
> > > Presumably the code that knows where MANIFEST.json is expected to be is
under our control so we could just change that? (If there's some upstream code
that uses the location of MANIFEST.json, that'd be a problem, but we could
change that too probably if it's the right thing.)
> > 
> > This sounds reasonable to me -- I'd be fine with this change in this CL or a
follow-up CL if you think it's a good idea. The manifest generator script takes
a --path argument which should be the path to the manifest file. The
_ensure_manifest method would still copy the base over to the manifest path and
then run the manifest generator command.
> > 
> > If this is done, the knowledge about where the manifest file is should be
kept in one place, probably webkitpy/w3c/wpt_manifest.py.
> > 
> >
https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
> > File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py
(right):
> > 
> >
https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
> >
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:102:
# Regenerate MANIFEST.json from template, necessary for WPT metadata
> > Nit: period at end of comment. Could also spell out web-platform-tests,
optionally.
> > 
> >
https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
> >
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:556:
def _ensure_manifest(self):
> > Do you think it makes sense to make this a public static method or top-level
function in webkitpy/w3c/wpt_manifest.py?
> > 
> >
https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
> >
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:558:
'LayoutTests', 'external', 'wpt', 'MANIFEST.json')
> > Optional idea: These paths relative to webkit base could potentially be
constants in webkitpy/w3c/wpt_manifest.py.
> > 
> >
https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
> >
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:560:
'LayoutTests', 'external', 'WPT_BASE_MANIFEST.json')
> > Here we're assuming that WPT_BASE_MANIFEST.json always exists.
> > This is fine, although you could also make this explicit:
> > 
> >     assert self._filesystem.exists(manifest_base_path)
> > 
> > Or, you could even make it just log an error and generate the whole manifest
from scratch anyway.
> > 
> >
https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
> > File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py
(right):
> > 
> >
https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
> >
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py:181:

> > As a convention, when explicitly testing protected methods in unit tests,
I've been adding a comment above the test methods like:
> > 
> >     # Tests for protected methods - pylint: disable=protected-access
> > 
> > To avoid having to repeat the pylint disable comment below. Does this sound
like an OK idea? I think that ideally one should test public methods first and
not necessarily test protected/private methods, but often having tests for
private methods is better than no tests, and in this case I think explicitly
saying this with a pylint disable comment is fine.
> > 
> >
https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
> >
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py:184:
port = host.port_factory.get('test-mac-mac10.10')
> > If the test is not asserting anything that's port-specific, you can also do
`port = host.port_factory.get()`, which for MockHost I think should return a
TestPort of some kind by default.
> > 
> >
https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
> >
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py:190:
self.assertEqual(port.host.executive.calls, [
> > On 2017/02/28 at 04:49:17, foolip wrote:
> > > I don't understand this assert, leaving it to qyearsley@ to understand :)
> > 
> > This is asserting that one subprocess was invoked, which is the manifest
generator script; this seems like what we want :-)
> > 
> >
https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
> >
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py:193:
'--work', '--tests-root',
'/mock-checkout/third_party/WebKit/LayoutTests/external/wpt']])
> > Nit: Optionally, you could also format this as something like:
> > 
> > self.assertEqual(
> >     port.host.executive.calls,
> >     [
> >         [
> >             'python',
> >             webkit_base +
'/Tools/Scripts/webkitpy/thirdparty/wpt/wpt/manifest',
> >             '--work',
> >             '--tests-root',
> >             webkit_base + '/LayoutTests/external/wpt',
> >         ]
> >     ])
> 
> Question before I send my individual feedback responses: it looks like
WPT_BASE_MANIFEST.json is too big for Rietveld (it's showing -0 +0 lines changed
and no diff). Will it be included in this commit or should I move this code
review to Gerrit?

I uploaded the same CL (with some modifications based on your feedback) to
Gerrit and WPT_BASE_MANIFEST.json is showing up (even though it takes a while to
load): https://chromium-review.googlesource.com/c/447959/

Should we continue this review on Gerrit to make sure that file gets committed?

[qyearsley, 2017-02-28 21:48:13]

On 2017/02/28 at 21:46:06, jeffcarp wrote:
> On 2017/02/28 at 21:18:46, jeffcarp wrote:
> > On 2017/02/28 at 18:54:45, qyearsley wrote:
> > >
https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
> > > File third_party/WebKit/Tools/Scripts/webkitpy/common/host_mock.py
(right):
> > > 
> > >
https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
> > > third_party/WebKit/Tools/Scripts/webkitpy/common/host_mock.py:86:
'LayoutTests', 'external', 'WPT_BASE_MANIFEST.json')
> > > On 2017/02/28 at 04:49:17, foolip wrote:
> > > > Had an idea while looking at this line. Would it be possible to put the
generated MANIFEST.json in the same dir next to this to trivially get around it
including itself? Or even better, somewhere in the out/ directory?
> > > >
> > > > Presumably the code that knows where MANIFEST.json is expected to be is
under our control so we could just change that? (If there's some upstream code
that uses the location of MANIFEST.json, that'd be a problem, but we could
change that too probably if it's the right thing.)
> > > 
> > > This sounds reasonable to me -- I'd be fine with this change in this CL or
a follow-up CL if you think it's a good idea. The manifest generator script
takes a --path argument which should be the path to the manifest file. The
_ensure_manifest method would still copy the base over to the manifest path and
then run the manifest generator command.
> > > 
> > > If this is done, the knowledge about where the manifest file is should be
kept in one place, probably webkitpy/w3c/wpt_manifest.py.
> > > 
> > >
https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
> > > File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py
(right):
> > > 
> > >
https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
> > >
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:102:
# Regenerate MANIFEST.json from template, necessary for WPT metadata
> > > Nit: period at end of comment. Could also spell out web-platform-tests,
optionally.
> > > 
> > >
https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
> > >
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:556:
def _ensure_manifest(self):
> > > Do you think it makes sense to make this a public static method or
top-level function in webkitpy/w3c/wpt_manifest.py?
> > > 
> > >
https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
> > >
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:558:
'LayoutTests', 'external', 'wpt', 'MANIFEST.json')
> > > Optional idea: These paths relative to webkit base could potentially be
constants in webkitpy/w3c/wpt_manifest.py.
> > > 
> > >
https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
> > >
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:560:
'LayoutTests', 'external', 'WPT_BASE_MANIFEST.json')
> > > Here we're assuming that WPT_BASE_MANIFEST.json always exists.
> > > This is fine, although you could also make this explicit:
> > > 
> > >     assert self._filesystem.exists(manifest_base_path)
> > > 
> > > Or, you could even make it just log an error and generate the whole
manifest from scratch anyway.
> > > 
> > >
https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
> > > File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py
(right):
> > > 
> > >
https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
> > >
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py:181:

> > > As a convention, when explicitly testing protected methods in unit tests,
I've been adding a comment above the test methods like:
> > > 
> > >     # Tests for protected methods - pylint: disable=protected-access
> > > 
> > > To avoid having to repeat the pylint disable comment below. Does this
sound like an OK idea? I think that ideally one should test public methods first
and not necessarily test protected/private methods, but often having tests for
private methods is better than no tests, and in this case I think explicitly
saying this with a pylint disable comment is fine.
> > > 
> > >
https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
> > >
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py:184:
port = host.port_factory.get('test-mac-mac10.10')
> > > If the test is not asserting anything that's port-specific, you can also
do `port = host.port_factory.get()`, which for MockHost I think should return a
TestPort of some kind by default.
> > > 
> > >
https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
> > >
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py:190:
self.assertEqual(port.host.executive.calls, [
> > > On 2017/02/28 at 04:49:17, foolip wrote:
> > > > I don't understand this assert, leaving it to qyearsley@ to understand
:)
> > > 
> > > This is asserting that one subprocess was invoked, which is the manifest
generator script; this seems like what we want :-)
> > > 
> > >
https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
> > >
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py:193:
'--work', '--tests-root',
'/mock-checkout/third_party/WebKit/LayoutTests/external/wpt']])
> > > Nit: Optionally, you could also format this as something like:
> > > 
> > > self.assertEqual(
> > >     port.host.executive.calls,
> > >     [
> > >         [
> > >             'python',
> > >             webkit_base +
'/Tools/Scripts/webkitpy/thirdparty/wpt/wpt/manifest',
> > >             '--work',
> > >             '--tests-root',
> > >             webkit_base + '/LayoutTests/external/wpt',
> > >         ]
> > >     ])
> > 
> > Question before I send my individual feedback responses: it looks like
WPT_BASE_MANIFEST.json is too big for Rietveld (it's showing -0 +0 lines changed
and no diff). Will it be included in this commit or should I move this code
review to Gerrit?
> 
> I uploaded the same CL (with some modifications based on your feedback) to
Gerrit and WPT_BASE_MANIFEST.json is showing up (even though it takes a while to
load): https://chromium-review.googlesource.com/c/447959/
> 
> Should we continue this review on Gerrit to make sure that file gets
committed?

Continuing review on Gerrit SGTM. Gerrit is the way of the future!

[jeffcarp, 2017-02-28 21:57:39]

Publishing my drafts but any subsequent review should be on Gerrit:
https://chromium-review.googlesource.com/c/447959

https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py
(right):

https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:102:
# Regenerate MANIFEST.json from template, necessary for WPT metadata
On 2017/02/28 at 18:54:45, qyearsley wrote:
> Nit: period at end of comment. Could also spell out web-platform-tests,
optionally.

Done

https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:556:
def _ensure_manifest(self):
On 2017/02/28 at 18:54:45, qyearsley wrote:
> Do you think it makes sense to make this a public static method or top-level
function in webkitpy/w3c/wpt_manifest.py?

My one reservation with that is that this method is very layout tests-specific.
But the manifest-related logic should be all in one place, so wpt_manifest.py
makes sense. Could this be in a follow-up CL?

https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:560:
'LayoutTests', 'external', 'WPT_BASE_MANIFEST.json')
On 2017/02/28 at 18:54:44, qyearsley wrote:
> Here we're assuming that WPT_BASE_MANIFEST.json always exists.
> This is fine, although you could also make this explicit:
> 
>     assert self._filesystem.exists(manifest_base_path)
> 
> Or, you could even make it just log an error and generate the whole manifest
from scratch anyway.

I think it's OK to assume it always exists because it will be checked into
source control. To make sure that is always the case, I could write some sort of
sanity check or PRESUBMIT to make sure it's there. But this is the same
assumption we're making now on master with respect to MANIFEST.json.

https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py
(right):

https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py:181:

On 2017/02/28 at 18:54:45, qyearsley wrote:
> As a convention, when explicitly testing protected methods in unit tests, I've
been adding a comment above the test methods like:
> 
>     # Tests for protected methods - pylint: disable=protected-access
> 
> To avoid having to repeat the pylint disable comment below. Does this sound
like an OK idea? I think that ideally one should test public methods first and
not necessarily test protected/private methods, but often having tests for
private methods is better than no tests, and in this case I think explicitly
saying this with a pylint disable comment is fine.

That sounds good. I'm testing the private method instead of testing
Manager.run() because the latter required a lot more boilerplate and setup.

https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py:184:
port = host.port_factory.get('test-mac-mac10.10')
On 2017/02/28 at 18:54:45, qyearsley wrote:
> If the test is not asserting anything that's port-specific, you can also do
`port = host.port_factory.get()`, which for MockHost I think should return a
TestPort of some kind by default.

Done

https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py:193:
'--work', '--tests-root',
'/mock-checkout/third_party/WebKit/LayoutTests/external/wpt']])
On 2017/02/28 at 18:54:45, qyearsley wrote:
> Nit: Optionally, you could also format this as something like:
> 
> self.assertEqual(
>     port.host.executive.calls,
>     [
>         [
>             'python',
>             webkit_base +
'/Tools/Scripts/webkitpy/thirdparty/wpt/wpt/manifest',
>             '--work',
>             '--tests-root',
>             webkit_base + '/LayoutTests/external/wpt',
>         ]
>     ])

Done

[qyearsley, 2017-02-28 22:04:31]

https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py
(right):

https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:556:
def _ensure_manifest(self):
On 2017/02/28 at 21:57:39, jeffcarp wrote:
> On 2017/02/28 at 18:54:45, qyearsley wrote:
> > Do you think it makes sense to make this a public static method or top-level
function in webkitpy/w3c/wpt_manifest.py?
> 
> My one reservation with that is that this method is very layout
tests-specific. But the manifest-related logic should be all in one place, so
wpt_manifest.py makes sense. Could this be in a follow-up CL?

Yep, follow-up CL would be fine, definitely.

https://codereview.chromium.org/2644783003/diff/250001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:560:
'LayoutTests', 'external', 'WPT_BASE_MANIFEST.json')
On 2017/02/28 at 21:57:39, jeffcarp wrote:
> On 2017/02/28 at 18:54:44, qyearsley wrote:
> > Here we're assuming that WPT_BASE_MANIFEST.json always exists.
> > This is fine, although you could also make this explicit:
> > 
> >     assert self._filesystem.exists(manifest_base_path)
> > 
> > Or, you could even make it just log an error and generate the whole manifest
from scratch anyway.
> 
> I think it's OK to assume it always exists because it will be checked into
source control. To make sure that is always the case, I could write some sort of
sanity check or PRESUBMIT to make sure it's there. But this is the same
assumption we're making now on master with respect to MANIFEST.json.

Alright; optionally you could still add an assert

[awdf, 2017-03-02 15:56:59]

drive-by comment: Please could you also update the docs at
https://chromium.googlesource.com/chromium/src/+/master/docs/testing/web_plat...
? 

I understand with this CL the command to generate MANIFEST won't be required,
which will be great!

Right now it's additionally confusing because the command in the docs looks like
it should be run from third_party/WebKit/, (as it starts with
Tools/Scripts/...), but actually needs to be run from src/ to avoid a 
"KeyError: './MANIFEST.json'".

[awdf, 2017-03-02 16:00:17]

On 2017/03/02 15:56:59, awdf wrote:
> drive-by comment: Please could you also update the docs at
>
https://chromium.googlesource.com/chromium/src/+/master/docs/testing/web_plat...
> ? 
> 
> I understand with this CL the command to generate MANIFEST won't be required,
> which will be great!
> 
> Right now it's additionally confusing because the command in the docs looks
like
> it should be run from third_party/WebKit/, (as it starts with
> Tools/Scripts/...), but actually needs to be run from src/ to avoid a 
> "KeyError: './MANIFEST.json'".

Or rather, it *looks* like it worked from src/, but actually it doesn't! And I
still get a KeyError from third_party/WebKit :(

I'll try a rebase, sorry for the distraction.

[awdf, 2017-03-02 16:21:45]

On 2017/03/02 16:00:17, awdf wrote:
> On 2017/03/02 15:56:59, awdf wrote:
> > drive-by comment: Please could you also update the docs at
> >
>
https://chromium.googlesource.com/chromium/src/+/master/docs/testing/web_plat...
> > ? 
> > 
> > I understand with this CL the command to generate MANIFEST won't be
required,
> > which will be great!
> > 
> > Right now it's additionally confusing because the command in the docs looks
> like
> > it should be run from third_party/WebKit/, (as it starts with
> > Tools/Scripts/...), but actually needs to be run from src/ to avoid a 
> > "KeyError: './MANIFEST.json'".
> 
> Or rather, it *looks* like it worked from src/, but actually it doesn't! And I
> still get a KeyError from third_party/WebKit :(
> 
> I'll try a rebase, sorry for the distraction.

..deleting the MANIFEST.json and rerunning the script (from third_party/WebKit/)
made it work, for the record.

Looking forward to when this patch lands and I don't need to worry about running
a script for it to pick up on my tests! :)

[jeffcarp, 2017-03-02 19:30:36]

On 2017/03/02 at 16:21:45, awdf wrote:
> On 2017/03/02 16:00:17, awdf wrote:
> > On 2017/03/02 15:56:59, awdf wrote:
> > > drive-by comment: Please could you also update the docs at
> > >
> >
https://chromium.googlesource.com/chromium/src/+/master/docs/testing/web_plat...
> > > ? 
> > > 
> > > I understand with this CL the command to generate MANIFEST won't be
required,
> > > which will be great!
> > > 
> > > Right now it's additionally confusing because the command in the docs
looks
> > like
> > > it should be run from third_party/WebKit/, (as it starts with
> > > Tools/Scripts/...), but actually needs to be run from src/ to avoid a 
> > > "KeyError: './MANIFEST.json'".
> > 
> > Or rather, it *looks* like it worked from src/, but actually it doesn't! And
I
> > still get a KeyError from third_party/WebKit :(
> > 
> > I'll try a rebase, sorry for the distraction.
> 
> ..deleting the MANIFEST.json and rerunning the script (from
third_party/WebKit/) made it work, for the record.
> 
> Looking forward to when this patch lands and I don't need to worry about
running a script for it to pick up on my tests! :)

Sorry you had to run into that! There's a bug for MANIFEST.json regeneration
being broken on ToT:
https://bugs.chromium.org/p/chromium/issues/detail?id=697207

The new review is here https://chromium-review.googlesource.com/c/447959, going
to try to land it today.