Bug 42832 - webkit-patch command to find the ports covering a specific layout test
Summary: webkit-patch command to find the ports covering a specific layout test
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-07-22 09:21 PDT by Philippe Normand
Modified: 2011-11-02 21:53 PDT (History)
5 users (show)

See Also:


Attachments
proposed patch (3.71 KB, patch)
2010-07-22 09:24 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
proposed patch (3.71 KB, patch)
2010-07-22 09:30 PDT, Philippe Normand
ojan: review-
Details | Formatted Diff | Diff
proposed patch (9.53 KB, patch)
2010-07-23 03:25 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
proposed patch (9.54 KB, patch)
2010-07-23 03:36 PDT, Philippe Normand
abarth: review-
Details | Formatted Diff | Diff
proposed patch (reworked) (17.54 KB, patch)
2010-09-06 10:12 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
proposed patch (reworked) (17.53 KB, patch)
2010-09-06 10:17 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
proposed patch (reworked) (17.60 KB, patch)
2010-09-06 10:21 PDT, Philippe Normand
abarth: review-
Details | Formatted Diff | Diff
updated patch (18.32 KB, patch)
2010-09-07 00:58 PDT, Philippe Normand
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2010-07-22 09:21:41 PDT
I needed such command so maybe some people would be interested too. Will attach the patch
Comment 1 Philippe Normand 2010-07-22 09:24:49 PDT
Created attachment 62307 [details]
proposed patch
Comment 2 WebKit Review Bot 2010-07-22 09:26:28 PDT
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.
Comment 3 Philippe Normand 2010-07-22 09:30:24 PDT
Created attachment 62309 [details]
proposed patch
Comment 4 Ojan Vafai 2010-07-22 12:17:08 PDT
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.
Comment 5 Philippe Normand 2010-07-23 00:43:57 PDT
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.
Comment 6 Philippe Normand 2010-07-23 03:25:31 PDT
Created attachment 62404 [details]
proposed patch
Comment 7 WebKit Review Bot 2010-07-23 03:27:44 PDT
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.
Comment 8 Philippe Normand 2010-07-23 03:36:15 PDT
Created attachment 62405 [details]
proposed patch
Comment 9 Adam Barth 2010-07-24 08:43:31 PDT
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.
Comment 10 Eric Seidel (no email) 2010-07-24 08:49:59 PDT
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.
Comment 11 Philippe Normand 2010-09-03 02:47:56 PDT
(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?
Comment 12 Eric Seidel (no email) 2010-09-03 11:07:42 PDT
(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.
Comment 13 Philippe Normand 2010-09-06 10:12:42 PDT
Created attachment 66653 [details]
proposed patch (reworked)
Comment 14 WebKit Review Bot 2010-09-06 10:13:39 PDT
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.
Comment 15 Philippe Normand 2010-09-06 10:17:30 PDT
Created attachment 66655 [details]
proposed patch (reworked)
Comment 16 WebKit Review Bot 2010-09-06 10:18:59 PDT
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.
Comment 17 Philippe Normand 2010-09-06 10:21:58 PDT
Created attachment 66656 [details]
proposed patch (reworked)

Sorry for the noise.
Comment 18 Adam Barth 2010-09-06 10:30:36 PDT
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.
Comment 19 Philippe Normand 2010-09-07 00:55:17 PDT
(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),
Comment 20 Philippe Normand 2010-09-07 00:58:51 PDT
Created attachment 66689 [details]
updated patch
Comment 21 Adam Barth 2010-09-07 01:00:01 PDT
(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.
Comment 22 Adam Barth 2010-09-07 01:01:32 PDT
Comment on attachment 66689 [details]
updated patch

Looks great.  Thanks.
Comment 23 Philippe Normand 2010-09-07 01:04:44 PDT
(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 :)
Comment 24 Adam Barth 2010-09-07 01:10:44 PDT
> > > > (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.  :)
Comment 25 Philippe Normand 2010-09-07 02:54:33 PDT
Landed in http://trac.webkit.org/changeset/66872 with little updates to make the unittests actually pass.
Comment 26 Philippe Normand 2010-09-07 03:14:04 PDT
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.
Comment 27 WebKit Review Bot 2010-09-07 04:10:26 PDT
http://trac.webkit.org/changeset/66872 might have broken SnowLeopard Intel Release (Tests)
Comment 28 Dirk Pranke 2010-09-14 19:17:29 PDT
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.
Comment 29 Adam Barth 2010-09-14 22:01:49 PDT
> 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.
Comment 30 Dirk Pranke 2010-09-15 14:25:51 PDT
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 ...
Comment 31 Adam Barth 2010-09-15 14:28:08 PDT
(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.  :)
Comment 32 Eric Seidel (no email) 2010-09-15 14:30:01 PDT
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. :)
Comment 33 Dirk Pranke 2010-09-15 14:31:26 PDT
(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.
Comment 34 Adam Barth 2010-09-15 14:36:06 PDT
> 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.
Comment 35 Eric Seidel (no email) 2010-09-15 14:40:15 PDT
Filed bug 45838.
Comment 36 Dirk Pranke 2010-10-11 18:12:59 PDT
I filed bug 47528 to fix the layering violations ...
Comment 37 Eric Seidel (no email) 2011-11-02 21:53:05 PDT
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'