Bug 92398 – All ports should support per test switching of pixel testing

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

[Balazs Kelemen]

This is the next step after bug 91754. The goal is to be able to specify what tests a platform want to run as pixel test with --pixel-test-directory. We will also be able to remove the DriverProxy hack in nrwt which was added to support reftests. I am going to remove the --pixel-tests option from DRT's because it's unlikely that anyone rely on the ability to use it manually. Please notify me if you don't agree on that.

[Balazs Kelemen]

Created attachment 154700 [details]
Patch

[Dirk Pranke]

Comment on attachment 154700 [details]
Patch

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

Thanks for changing all of the ports! that's great. That said, R-'ing for the typo on the mac port :)

Also, it might be nice if someone else who was more familiar with the DRT/WTR/C++ code took a look. Tony, what do you think?

In your earlier change, I missed that the format for the line written to stdin was "test'--pixel-test'hash" ... I was hoping to extend this to support arbitrary arguments, which I suppose you can do by just splicing them between single quotes, but it seems a little odd. why not just separate the args with spaces, e.g., test hash --pixel-test --foo --bar, etc.?

> Tools/DumpRenderTree/efl/DumpRenderTree.cpp:232
> +static bool getExpectedPixelHash(const String& testURL, String& pixelHashOut)

I don't know the EFL implementation at all, but (perhaps naively) wouldn't it be better to parse the url in the caller to see if you needed to get the hash? Or maybe at least change the name of this function?

> Tools/DumpRenderTree/mac/DumpRenderTree.mm:876
> +        printSeparators = (optind < argc - 1 || (dumpPixelsForCurrentTest && dumpTree);

does this compile? I think you're missing a closing paren ...

[Tony Chang]

Comment on attachment 154700 [details]
Patch

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

It's very brave of you to try to change all the ports at the same time! :)

It seems like the code for parsing pathOrURL and the command line arguments is the same for all DRTs (except maybe EFL).  Can we move that into a helper file?  Something like CyclicRedundancyCheck.h or PixelDumpSupport.h.

This was probably discussed in an earlier bug, but why is ' used as a separator? Can't we just search for -- after a space?

> Tools/ChangeLog:10
> +        on the standart input that means that running the current

typo: standart -> standard

> Tools/Scripts/webkitpy/layout_tests/port/driver.py:342
> +            command += "'" + '--pixel-test'

Nit: Can this just be command += "'--pixel-test" ?

[Balazs Kelemen]

(In reply to comment #3)
> (From update of attachment 154700 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154700&action=review
> 
> It's very brave of you to try to change all the ports at the same time! :)
> 

I know... :)

> It seems like the code for parsing pathOrURL and the command line arguments is the same for all DRTs (except maybe EFL).  Can we move that into a helper file?  Something like CyclicRedundancyCheck.h or PixelDumpSupport.h.
> 
> This was probably discussed in an earlier bug, but why is ' used as a separator? Can't we just search for -- after a space?

Both are good idea. I will do that way in the next patch.

[Balazs Kelemen]

> 
> In your earlier change, I missed that the format for the line written to stdin was "test'--pixel-test'hash" ... I was hoping to extend this to support arbitrary arguments, which I suppose you can do by just splicing them between single quotes, but it seems a little odd. why not just separate the args with spaces, e.g., test hash --pixel-test --foo --bar, etc.?
> 

I have tried to implement this but than realized that ' as separator is used because spaces are used to separate paths (which are simply the argv array in when running in standalone mode). I don't think it's worth to add a bunch of command line parsing logic to drt.

[Balazs Kelemen]

Created attachment 155320 [details]
patch for ews's

[Early Warning System Bot]

Comment on attachment 155320 [details]
patch for ews's

Attachment 155320 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13396347

[Early Warning System Bot]

Comment on attachment 155320 [details]
patch for ews's

Attachment 155320 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13394378

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/DumpRenderTree/D..." exit_code: 1
Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:34:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
Total errors found: 1 in 20 files


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

[Build Bot]

Comment on attachment 155320 [details]
patch for ews's

Attachment 155320 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13387457

[Dirk Pranke]

(In reply to comment #5)
> I have tried to implement this but than realized that ' as separator is used because spaces are used to separate paths (which are simply the argv array in when running in standalone mode). I don't think it's worth to add a bunch of command line parsing logic to drt.

DRT supports running more than one test at a time using argv? News to me; do all ports support that?

[Gyuyoung Kim]

Comment on attachment 155320 [details]
patch for ews's

Attachment 155320 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13385588

[Balazs Kelemen]

(In reply to comment #11)
> (In reply to comment #5)
> > I have tried to implement this but than realized that ' as separator is used because spaces are used to separate paths (which are simply the argv array in when running in standalone mode). I don't think it's worth to add a bunch of command line parsing logic to drt.
> 
> DRT supports running more than one test at a time using argv? News to me; do all ports support that?

Yes, all of them.

[Balazs Kelemen]

Created attachment 155325 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 155325 [details]
Patch

Attachment 155325 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13385598

[Build Bot]

Comment on attachment 155325 [details]
Patch

Attachment 155325 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13381862

[WebKit Review Bot]

Comment on attachment 155325 [details]
Patch

Attachment 155325 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13391413

[Tony Chang]

(In reply to comment #5)
> > 
> > In your earlier change, I missed that the format for the line written to stdin was "test'--pixel-test'hash" ... I was hoping to extend this to support arbitrary arguments, which I suppose you can do by just splicing them between single quotes, but it seems a little odd. why not just separate the args with spaces, e.g., test hash --pixel-test --foo --bar, etc.?
> > 
> 
> I have tried to implement this but than realized that ' as separator is used because spaces are used to separate paths (which are simply the argv array in when running in standalone mode). I don't think it's worth to add a bunch of command line parsing logic to drt.

I still don't see why you need '.  Any token starting with -- is a flag; everything else is a filename/URL.

[Balazs Kelemen]

> 
> I still don't see why you need '.  Any token starting with -- is a flag; everything else is a filename/URL.

I did not say it's not possible to get rid of it. I said I don't want to bother with it. I don't make it worst than it was and improving it is out of the scope of this patch. (And anyway, it's not a common use case when one want to pass -p and the hash manually.)

[Balazs Kelemen]

Created attachment 155474 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 155474 [details]
Patch

Attachment 155474 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13388723

[WebKit Review Bot]

Comment on attachment 155474 [details]
Patch

Attachment 155474 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13384893

[Build Bot]

Comment on attachment 155474 [details]
Patch

Attachment 155474 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13386787

[Early Warning System Bot]

Comment on attachment 155474 [details]
Patch

Attachment 155474 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13387710

[Early Warning System Bot]

Comment on attachment 155474 [details]
Patch

Attachment 155474 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13385847

[Balazs Kelemen]

Created attachment 155489 [details]
Patch

[WebKit Review Bot]

Comment on attachment 155489 [details]
Patch

Attachment 155489 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13400395

[Build Bot]

Comment on attachment 155489 [details]
Patch

Attachment 155489 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13400410

[Balazs Kelemen]

Created attachment 155513 [details]
Patch

[WebKit Review Bot]

Comment on attachment 155513 [details]
Patch

Attachment 155513 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13390754

[Balazs Kelemen]

Created attachment 155519 [details]
Patch

[WebKit Review Bot]

Comment on attachment 155519 [details]
Patch

Attachment 155519 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13385931

[Balazs Kelemen]

Could somebody tell me how can I include the common DumpRenderTree.h in the chromium DumpRenderTree.cpp? Include path is not set, #include "../DumpRenderTree.h" doesn't work. I don't know how to add include paths in the gpy build. Could you catch me on IRC? Thanks!

[Balazs Kelemen]

Created attachment 155569 [details]
Patch

[Build Bot]

Comment on attachment 155569 [details]
Patch

Attachment 155569 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13410019

[Balazs Kelemen]

Created attachment 155580 [details]
Patch

[WebKit Review Bot]

Comment on attachment 155580 [details]
Patch

Attachment 155580 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13403260

[Dirk Pranke]

Comment on attachment 155580 [details]
Patch

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

r+ with the nit that I'd change the delimiter back to ' if we're not going to use spaces. I assume you'd have no complaints with someone uploading a separate patch to switch to spaces? Also, fix the .gyp file, please ;).

Tony, are you okay with this?

> Tools/DumpRenderTree/DumpRenderTree.gyp/DumpRenderTree.gyp:136
> +                '../'

you probably need a comma here, else I expect this is messing up the compile path: you'd get '../<(DEPTH)' :). 

Also, this should probably be '<(tools_dir)/DumpRenderTree' rather than a relative path.

> Tools/Scripts/old-run-webkit-tests:754
> +        $suffixPixelTest = ":--pixel-test";

if we're not able to use spaces, it seems like we might as well stay with single quotes as the delimiter.

> Tools/Scripts/webkitpy/layout_tests/port/driver.py:339
> +            command += ":" + driver_input.image_hash

as above.

[Tony Chang]

Comment on attachment 155580 [details]
Patch

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

>> Tools/DumpRenderTree/DumpRenderTree.gyp/DumpRenderTree.gyp:136
>> +                '../'
> 
> you probably need a comma here, else I expect this is messing up the compile path: you'd get '../<(DEPTH)' :). 
> 
> Also, this should probably be '<(tools_dir)/DumpRenderTree' rather than a relative path.

Heh, I suggested the relative path.  Either is fine.

> Tools/DumpRenderTree/DumpRenderTreeCommon.cpp:12
> +    CommandTokenizer(const std::string& input)

Nit: explicit

> Tools/DumpRenderTree/DumpRenderTreeCommon.cpp:53
> +    return m_next != std::string();

Nit: !m_next.empty()

> Tools/WebKitTestRunner/TestController.cpp:516
> +    CommandTokenizer(const std::string& input)

explicit

> Tools/WebKitTestRunner/TestController.cpp:557
> +    return m_next != std::string();

!m_next.empty()

[Dirk Pranke]

(In reply to comment #39)
> > 
> > Also, this should probably be '<(tools_dir)/DumpRenderTree' rather than a relative path.
> 
> Heh, I suggested the relative path.  Either is fine.
>

I've found using relative paths to be fragile and confusing, especially when trying to move files around and/or sort through webkit-in-chromium vs. chromium-in-webkit checkouts. Using the variables made things a lot clearer (again, to me).

[Balazs Kelemen]

Landed in http://trac.webkit.org/changeset/124313.
Keeping bug open because I forgot to remove --pixel-tests from the Qt DRT.

[Balazs Kelemen]

Need to roll out. Adding Tools/DumpRenderTree as include path does not work because there are some files with the same name in the chromium dir. Do you agree to rename these with a Chromium suffix? It would be:
AccessibilityController.h/cpp -> AccessibilityControllerChromium.h/cpp
AccessibilityUIElement.h/cpp -> AccessibilityUIElementChromium.h/cpp

[Balazs Kelemen]

(In reply to comment #42)
> Need to roll out. Adding Tools/DumpRenderTree as include path does not work because there are some files with the same name in the chromium dir. Do you agree to rename these with a Chromium suffix? It would be:
> AccessibilityController.h/cpp -> AccessibilityControllerChromium.h/cpp
> AccessibilityUIElement.h/cpp -> AccessibilityUIElementChromium.h/cpp

Actually we could also change the includes, like
-#include "AccessibilityController.h"
+#include "chromium/TestRunner/AccessibilityController.h"

Which solution do you prefer? :)

[Adam Barth]

> Actually we could also change the includes, like
> -#include "AccessibilityController.h"
> +#include "chromium/TestRunner/AccessibilityController.h"
> 
> Which solution do you prefer? :)

Using an include path like "chromium/TestRunner/AccessibilityController.h" in WebKit isn't correct.

[Dirk Pranke]

Perhaps we should just copy DumpRenderTree.h down into the chromium dir ... it looks like that's what we've done (more or less) for other files. I don't like the duplication, but sharing more code properly is probably a bigger effort than what should be required to make this patch land.

[Balazs Kelemen]

Created attachment 156011 [details]
Patch

[Balazs Kelemen]

(In reply to comment #45)
> Perhaps we should just copy DumpRenderTree.h down into the chromium dir ... it looks like that's what we've done (more or less) for other files. I don't like the duplication, but sharing more code properly is probably a bigger effort than what should be required to make this patch land.

Well, I don't like this idea, I think renaming those files is much more straightforward, so I did that.

[Balazs Kelemen]

Wow, finally it builds everywhere :)

[Balazs Kelemen]

Comment on attachment 156011 [details]
Patch

Going to land it tomorrow as it is late here and I would like to watch bots.

[Balazs Kelemen]

Landed in r124581. ELF regression is fixed in r124595.

[Xianzhu Wang]

*** Bug 91538 has been marked as a duplicate of this bug. ***

[Simon Fraser (smfr)]

How can I manually run DRT with the path to a test and -p to dump pixels now?

[Dirk Pranke]

(In reply to comment #52)
> How can I manually run DRT with the path to a test and -p to dump pixels now?

Assuming your environment is set up properly, 

path-to-DumpRenderTree path-to-LayoutTests/foo/bar.html'-pixel-test

should work (just tested this earlier today).