Bug 42832 – webkit-patch command to find the ports covering a specific layout test

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Philippe Normand]

I needed such command so maybe some people would be interested too. Will attach the patch

[Philippe Normand]

Created attachment 62307 [details]
proposed patch

[WebKit Review Bot]

Attachment 62307 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKitTools/Scripts/webkitpy/tool/commands/queries.py:289:  expected 2 blank lines, found 1  [pep8/E302] [5]
WebKitTools/Scripts/webkitpy/tool/commands/queries.py:300:  whitespace after '['  [pep8/E201] [5]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Created attachment 62309 [details]
proposed patch

[Ojan Vafai]

Comment on attachment 62309 [details]
proposed patch

This seems like a useful command. This patch needs a unittest.

> +        webkit-patch command to find the ports covering a specific layout test

I don't think using the word "cover" is very clear. How about s/covering/not skipping/?

> +class WhatCovers(AbstractDeclarativeCommand):
> +    name = "what-covers"

This name isn't very clear. Instead of giving the list of ports that don't skip the test, how about returning the list of ports that do skip it. Usually, that list will be much shorter. Then this command could be called "skipped-ports" or something like htat.

> +    def execute(self, options, args, tool):
> +        platform_directory = os.path.join(tool.scm().cwd, "LayoutTests", "platform")

Ideally this wouldn't depend on being run from a specific directory. scm.py has some bits to find to root directory of the checkout.

> +        results = dict([(test, []) for test in args])
> +        for port_name in os.listdir(platform_directory):
> +            skipped_file = os.path.join(platform_directory, port_name, "Skipped")

Can you put a FIXME to also check LayoutTests/platform/chromium/test_expectations.txt? Or, even better, just add the code to parse it. That file has a different syntax. Each test is on it's own line, but it's not just skipped. The format for the line is:
MODIFIERS : path/to/test = EXPECTATIONS

If the test is skipped, the one of the MODIFIERS will be SKIP.

[Philippe Normand]

Thanks for the review. I made the requested changes but I need more time for the unittest as it requires a new mock and maybe a new API in scm.py, not sure yet.

[Philippe Normand]

Created attachment 62404 [details]
proposed patch

[WebKit Review Bot]

Attachment 62404 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKitTools/Scripts/webkitpy/tool/mocktool.py:427:  missing whitespace after ','  [pep8/E231] [5]
WebKitTools/Scripts/webkitpy/tool/mocktool.py:429:  missing whitespace after ','  [pep8/E231] [5]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Created attachment 62405 [details]
proposed patch

[Adam Barth]

Comment on attachment 62405 [details]
proposed patch

WebKitTools/Scripts/webkitpy/common/checkout/scm.py:244
 +      def directory_contents(self, path):
Please add unit tests in scm_unittests.  To run the SCM unit tests, you need to pass --all to test-webkitpy.

WebKitTools/Scripts/webkitpy/tool/commands/queries.py:301
 +                           if line.strip() and not line.startswith("#")]
This calls line.strip twice.  :(

WebKitTools/Scripts/webkitpy/tool/commands/queries.py:307
 +                      tests_skipped.append(test_name)
skipped_tests and tests_skipped are very similar names.  It's hard to know which is which.

WebKitTools/Scripts/webkitpy/tool/commands/queries.py:297
 +      def _find_tests_in_skipped(self, scm, tests, skipped_file):
We shouldn't be parsing Skipped files at the command layer.  That's something much lower level.  We need to add some facilities in one of the lower-level modules so that we can re-use this parsing code in other commands.

WebKitTools/Scripts/webkitpy/tool/commands/queries.py:319
 +          chromium_expectations_path = os.path.join(platform_directory, "chromium", "test_expectations.txt")
The command layer shouldn't know anything about specific ports.  All that information needs to be encapsulated in the port abstraction.

WebKitTools/Scripts/webkitpy/tool/commands/queries.py:321
 +                   if line.find("SKIP") >= 0 and line.strip() and not line.startswith("//")]
We already have a very nice facility for parsing test_expectations.  Please don't re-invent the wheel.

WebKitTools/Scripts/webkitpy/tool/commands/queries.py:350
 +          scm = tool.scm()
We don't generally store scm in a local variable.  We just access it off the tool.

WebKitTools/Scripts/webkitpy/tool/commands/queries.py:351
 +          platform_directory = tool.scm().absolute_path(os.path.join("LayoutTests", "platform"))
This knowledge doesn't belong at the command layer.

WebKitTools/Scripts/webkitpy/tool/mocktool.py:438
 +  # I am a Skipped file
This sort of fixture belongs in the unit test for a module that understands Skipped files.  These fixtures are a bit higher level.  That's just a symptom of trying to put too much at the command layer.

[Eric Seidel (no email)]

We also have some minimal skipped file parsing in layout_test/port/webkit.py iirc.  That too belongs pushed into common instead of the command layer.

[Philippe Normand]

(In reply to comment #10)
> We also have some minimal skipped file parsing in layout_test/port/webkit.py iirc.  That too belongs pushed into common instead of the command layer.

Why common? After looking at it the port layer seems like the way to go for me. The command could reuse it, right?

[Eric Seidel (no email)]

(In reply to comment #11)
> (In reply to comment #10)
> > We also have some minimal skipped file parsing in layout_test/port/webkit.py iirc.  That too belongs pushed into common instead of the command layer.
> 
> Why common? After looking at it the port layer seems like the way to go for me. The command could reuse it, right?

Do you mean putting the skipped file parsing in webkit.py?  That's fine.  I consider that "common" even though it's not in the "common" directory yet.

[Philippe Normand]

Created attachment 66653 [details]
proposed patch (reworked)

[WebKit Review Bot]

Attachment 66653 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKitTools/Scripts/webkitpy/layout_tests/port/factory.py:35:  missing whitespace around operator  [pep8/E225] [5]
WebKitTools/Scripts/webkitpy/layout_tests/port/factory.py:95:  expected 2 blank lines, found 1  [pep8/E302] [5]
WebKitTools/Scripts/webkitpy/layout_tests/port/webkit_unittest.py:44:  missing whitespace after ','  [pep8/E231] [5]
WebKitTools/Scripts/webkitpy/layout_tests/port/webkit_unittest.py:47:  missing whitespace after ','  [pep8/E231] [5]
WebKitTools/Scripts/webkitpy/tool/mocktool.py:558:  expected 2 blank lines, found 1  [pep8/E302] [5]
WebKitTools/Scripts/webkitpy/tool/mocktool.py:563:  expected 2 blank lines, found 1  [pep8/E302] [5]
WebKitTools/Scripts/webkitpy/tool/mocktool.py:568:  expected 2 blank lines, found 1  [pep8/E302] [5]
WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py:237:  whitespace after '['  [pep8/E201] [5]
WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py:108:  missing whitespace after ','  [pep8/E231] [5]
Total errors found: 9 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Created attachment 66655 [details]
proposed patch (reworked)

[WebKit Review Bot]

Attachment 66655 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKitTools/Scripts/webkitpy/layout_tests/port/factory.py:95:  expected 2 blank lines, found 1  [pep8/E302] [5]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Created attachment 66656 [details]
proposed patch (reworked)

Sorry for the noise.

[Adam Barth]

Comment on attachment 66656 [details]
proposed patch (reworked)

View in context: https://bugs.webkit.org/attachment.cgi?id=66656&action=prettypatch

Mostly nits.  The fake test_expectations parser is my main complaint.

> WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:317
> +            basename, extension = os.path.splitext(test_or_category)
> +            if not extension and test_name.startswith(basename):
> +                return True
Crazy.  We decide if something is a directory by whether it has an extension?  It feels like there should be a better way to do this, but I don't know what it is.

> WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py:239
> +    def skipped_layout_tests(self):
> +        return [line.split(':')[1].split("=")[0].strip()
> +                for line in self.test_expectations().splitlines()
> +                if line.find('SKIP') != -1 and not line.startswith('//')]
Surely we should use the test_expectation file parser instead of hacking one together here.

> WebKitTools/Scripts/webkitpy/layout_tests/port/factory.py:37
> +ALL_PORT_NAMES = ['test', 'dryrun', 'mac', 'win', 'gtk', 'qt', 'chromium-mac',
> +                  'chromium-linux', 'chromium-win', 'google-chrome-win',
> +                  'google-chrome-mac', 'google-chrome-linux32', 'google-chrome-linux64']
This seems like a maintenance burden.  Can we get this information by crawling the port classes that have been imported into memory?  You can see how we do that to autoregister commands.

> WebKitTools/Scripts/webkitpy/tool/commands/queries.py:298
> +        class Options:
> +            pass
This seems like a ugly pattern.  Can't we just pass an empty dictionary to represent no options?

> WebKitTools/Scripts/webkitpy/tool/main.py:80
> +        self.port_factory = port.factory
Nice.  I'm glad these two subsystems are finally getting integrated.

[Philippe Normand]

(In reply to comment #18)
> (From update of attachment 66656 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=66656&action=prettypatch
> 
> Mostly nits.  The fake test_expectations parser is my main complaint.
> 
> > WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:317
> > +            basename, extension = os.path.splitext(test_or_category)
> > +            if not extension and test_name.startswith(basename):
> > +                return True
> Crazy.  We decide if something is a directory by whether it has an extension?  It feels like there should be a better way to do this, but I don't know what it is.
> 

Crazy indeed! Will be fixed in next patch.

> > WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py:239
> > +    def skipped_layout_tests(self):
> > +        return [line.split(':')[1].split("=")[0].strip()
> > +                for line in self.test_expectations().splitlines()
> > +                if line.find('SKIP') != -1 and not line.startswith('//')]
> Surely we should use the test_expectation file parser instead of hacking one together here.
> 

Sure we can use that parser but it's really slow, at least here. Anyway, will make use of it in next patch...

> > WebKitTools/Scripts/webkitpy/layout_tests/port/factory.py:37
> > +ALL_PORT_NAMES = ['test', 'dryrun', 'mac', 'win', 'gtk', 'qt', 'chromium-mac',
> > +                  'chromium-linux', 'chromium-win', 'google-chrome-win',
> > +                  'google-chrome-mac', 'google-chrome-linux32', 'google-chrome-linux64']
> This seems like a maintenance burden.  Can we get this information by crawling the port classes that have been imported into memory?  You can see how we do that to autoregister commands.
> 

I don't see how to do that. The port modules are imported conditionally (see the get function in that same file, which is too BTW an interesting maintenance burden :)).

> > WebKitTools/Scripts/webkitpy/tool/commands/queries.py:298
> > +        class Options:
> > +            pass
> This seems like a ugly pattern.  Can't we just pass an empty dictionary to represent no options?
> 

The chromium and webkit port implementations require a class or instance object, see for instance:

webkit.py, line 368, in _build_path
    self.flag_from_configuration(self._options.configuration),

[Philippe Normand]

Created attachment 66689 [details]
updated patch

[Adam Barth]

(In reply to comment #19)
> (In reply to comment #18)
> > (From update of attachment 66656 [details] [details])
> > > WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py:239
> > > +    def skipped_layout_tests(self):
> > > +        return [line.split(':')[1].split("=")[0].strip()
> > > +                for line in self.test_expectations().splitlines()
> > > +                if line.find('SKIP') != -1 and not line.startswith('//')]
> > Surely we should use the test_expectation file parser instead of hacking one together here.
> 
> Sure we can use that parser but it's really slow, at least here. Anyway, will make use of it in next patch...

How slow are we talking about?  In general, we don't worry too much about performance of these commands.  Maintainability is usually more important.

[Adam Barth]

Comment on attachment 66689 [details]
updated patch

Looks great.  Thanks.

[Philippe Normand]

(In reply to comment #21)
> (In reply to comment #19)
> > (In reply to comment #18)
> > > (From update of attachment 66656 [details] [details] [details])
> > > > WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py:239
> > > > +    def skipped_layout_tests(self):
> > > > +        return [line.split(':')[1].split("=")[0].strip()
> > > > +                for line in self.test_expectations().splitlines()
> > > > +                if line.find('SKIP') != -1 and not line.startswith('//')]
> > > Surely we should use the test_expectation file parser instead of hacking one together here.
> > 
> > Sure we can use that parser but it's really slow, at least here. Anyway, will make use of it in next patch...
> 
> How slow are we talking about?  In general, we don't worry too much about performance of these commands.  Maintainability is usually more important.

It takes about 5 seconds to parse the expectations file for each chromium port instance used by the query. I think there's a bug opened about this, will try to find it back.

Thanks a lot for the reviews and your patience :)

[Adam Barth]

> > > > (From update of attachment 66656 [details] [details] [details] [details])
> > > > > WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py:239
> > > > > +    def skipped_layout_tests(self):
> > > > > +        return [line.split(':')[1].split("=")[0].strip()
> > > > > +                for line in self.test_expectations().splitlines()
> > > > > +                if line.find('SKIP') != -1 and not line.startswith('//')]
> > > > Surely we should use the test_expectation file parser instead of hacking one together here.
> > > 
> > > Sure we can use that parser but it's really slow, at least here. Anyway, will make use of it in next patch...
> > 
> > How slow are we talking about?  In general, we don't worry too much about performance of these commands.  Maintainability is usually more important.
> 
> It takes about 5 seconds to parse the expectations file for each chromium port instance used by the query. I think there's a bug opened about this, will try to find it back.

5 seconds is super long.  Thanks for the patch.  :)

[Philippe Normand]

Landed in http://trac.webkit.org/changeset/66872 with little updates to make the unittests actually pass.

[Philippe Normand]

Also commited http://trac.webkit.org/changeset/66873 after figuring out my checkout directory wasn't totally clean, which made the tests pass for me but not on the buildbots.

[WebKit Review Bot]

http://trac.webkit.org/changeset/66872 might have broken SnowLeopard Intel Release (Tests)

[Dirk Pranke]

Hrm.

The port layer is supposed to be the lowest layer in the system. It shouldn't be depending on anything from the layout_tests module, and definitely not the test_expectations package.

This is somewhat confusing because the webkit implementation conses up a fake expectations file from the Skipped files, and so it creates the appearance that the port knows something about expectations, but it really shouldn't (apart from how to find them).

If you look at the chromium implementation, you'll see that we only return the Release value, which means that if we skip a test in Debug mode, we won't notice it.

The functionality is useful, but there's probably a better way to do this. We probably actually want something like the way we do --lint-test-files in run_webkit_tests, which does exercise all of the possible configurations for a given port.

[Adam Barth]

> The port layer is supposed to be the lowest layer in the system. It shouldn't be depending on anything from the layout_tests module, and definitely not the test_expectations package.

Ok.  So it sounds like we need a new object that can depend both on port and on test expectations.

[Dirk Pranke]

curious why we tacked this onto webkit-patch rather than a separate script? It doesn't really have anything to do with SCM or managing patches ...

[Adam Barth]

(In reply to comment #30)
> curious why we tacked this onto webkit-patch rather than a separate script? It doesn't really have anything to do with SCM or managing patches ...

webkit-patch is a kitchen sink.  :)

[Eric Seidel (no email)]

webkit-patch has long been a catch-all for webkitpy scripts.  WE have a zillion hidden commands (including this one).

We have a long-standing bug where we need to separate out the "tool" data storage from WebKitPatch so that it's easier to write webkitpy-using commands.

Once we finally do that, we could easily create a separate kitchen-sink tool or folks could add other webkit-patch like tools for specific command themes.

See webkit-patch help --all-commands if you'd like to see the kitchen sink in its full glory. :)

[Dirk Pranke]

(In reply to comment #32)
> webkit-patch has long been a catch-all for webkitpy scripts.  WE have a zillion hidden commands (including this one).
> 
> We have a long-standing bug where we need to separate out the "tool" data storage from WebKitPatch so that it's easier to write webkitpy-using commands.
> 
> Once we finally do that, we could easily create a separate kitchen-sink tool or folks could add other webkit-patch like tools for specific command themes.
> 
> See webkit-patch help --all-commands if you'd like to see the kitchen sink in its full glory. :)

Is that bug actually filed? I'd be happy to take a look at it.

[Adam Barth]

> Is that bug actually filed? I'd be happy to take a look at it.

Go for it.  WARNING: You'll be entering the dungeon of code from when webkitpy was a single Python file and Eric had no clue that you didn't need curly brakets.

[Eric Seidel (no email)]

Filed bug 45838.

[Dirk Pranke]

I filed bug 47528 to fix the layering violations ...

[Eric Seidel (no email)]

It appears I broke this command a long time ago and no one noticed:

% webkit-patch skipped-ports svg                                                                                                                     [/Projects/WebKit/Tools/Scripts/webkitpy]
Traceback (most recent call last):
  File "/Projects/WebKit/Tools/Scripts/webkit-patch", line 74, in <module>
    main()
  File "/Projects/WebKit/Tools/Scripts/webkit-patch", line 69, in main
    WebKitPatch(os.path.abspath(__file__)).main()
  File "/Projects/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 308, in main
    result = command.check_arguments_and_execute(options, args, self)
  File "/Projects/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 117, in check_arguments_and_execute
    return self.execute(options, args, tool) or 0
  File "/Projects/WebKit/Tools/Scripts/webkitpy/tool/commands/queries.py", line 391, in execute
    for port_name, port_object in tool.port_factory.get_all().iteritems():
AttributeError: 'PortFactory' object has no attribute 'get_all'