Bug 81729 – REGRESSION: On Mac, run-webkit-tests changes the display color profile even when not running pixel tests

• Component: Tools / Tests
• Status: RESOLVED FIXED

run-webkit-tests changes the main display’s color profile when it’s run even if the --pixel option is not specified, so pixel tests aren’t run.

[Tim Horton]

This behavior matches Chromium, I believe (since I just stole their code), but we should fix it!

[Maciej Stachowiak]

Created attachment 139408 [details]
Patch

[Maciej Stachowiak]

Committed r115601: <http://trac.webkit.org/changeset/115601>

[Tim Horton]

I wonder if we should leave a note in the LayoutTestHelper code, in case anyone goes to add anything not-pixel-test-specific there. Or should rename LayoutTestHelper to PixelTestHelper or something.

[Tim Horton]

(In reply to comment #4)
> I wonder if we should leave a note in the LayoutTestHelper code, in case anyone goes to add anything not-pixel-test-specific there. Or should rename LayoutTestHelper to PixelTestHelper or something.

Woah, you said the same thing on webkit-dev. Nevermind!

[Rafael Brandao]

Comment on attachment 139408 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:851
> +            self._port.start_helper()

Wouldn't be a good idea to have this call named as "start_pixel_test_helper" instead?

[Dirk Pranke]

Comment on attachment 139408 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:851
>> +            self._port.start_helper()
> 
> Wouldn't be a good idea to have this call named as "start_pixel_test_helper" instead?

No. The intent of this method is to provide a hook for a port to do *whatever* customization is needed prior to a test run, not just pixel-test specific things.

In fact, this change would break the android port if it tried to run w/o -p, for example.

The correct thing to do would've been to change mac.py's implementation to be a no-op if options.pixel_test was false.

[Maciej Stachowiak]

(In reply to comment #7)
> (From update of attachment 139408 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139408&action=review
> 
> >> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:851
> >> +            self._port.start_helper()
> > 
> > Wouldn't be a good idea to have this call named as "start_pixel_test_helper" instead?
> 
> No. The intent of this method is to provide a hook for a port to do *whatever* customization is needed prior to a test run, not just pixel-test specific things.
> 
> In fact, this change would break the android port if it tried to run w/o -p, for example.
> 
> The correct thing to do would've been to change mac.py's implementation to be a no-op if options.pixel_test was false.

Maybe there should be separate hooks. The Chromium Android port that needs non-pixel-test setup isn't in webkit svn (so how could anyone know it exists, let alone when it needs to run?) while the three ports using this in open source (mac, chromium-mac, chromium-win) all need it only for pixel tests. Seems error prone to put the pixel test conditionality in each separate port.

[Dirk Pranke]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 139408 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=139408&action=review
> > 
> > >> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:851
> > >> +            self._port.start_helper()
> > > 
> > > Wouldn't be a good idea to have this call named as "start_pixel_test_helper" instead?
> > 
> > No. The intent of this method is to provide a hook for a port to do *whatever* customization is needed prior to a test run, not just pixel-test specific things.
> > 
> > In fact, this change would break the android port if it tried to run w/o -p, for example.
> > 
> > The correct thing to do would've been to change mac.py's implementation to be a no-op if options.pixel_test was false.
> 
> Maybe there should be separate hooks. The Chromium Android port that needs non-pixel-test setup isn't in webkit svn (so how could anyone know it exists, let alone when it needs to run?) 

The python code for it is:

http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py

> Seems error prone to put the pixel test conditionality in each separate port.

Perhaps. However, as stated above, the contract for the function was not pixel-test specific, so one had no choice given the current API.

[Hao Zheng]

(In reply to comment #7)
> (From update of attachment 139408 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139408&action=review
> 
> >> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:851
> >> +            self._port.start_helper()
> > 
> > Wouldn't be a good idea to have this call named as "start_pixel_test_helper" instead?
> 
> No. The intent of this method is to provide a hook for a port to do *whatever* customization is needed prior to a test run, not just pixel-test specific things.
> 
> In fact, this change would break the android port if it tried to run w/o -p, for example.
> 
> The correct thing to do would've been to change mac.py's implementation to be a no-op if options.pixel_test was false.

+1

Even if you want to make this change, you'd better change the name of the method to start_pixel_helper and change start_helper to setup_test_run in chromium_android. That would be a more sound change. This change actually breaks chromium android, although we don't run layout test publicly now as much stuff has not been upstreamed yet.

[Alexey Proskuryakov]

In addition to the above comments, this patch breaks reftests, as they are also dependent on color profile (they shouldn't be, but they are). I'm proposing to undo this change in bug 120755.