NRWT: option --skip-pixel-test-if-no-baseline support on DRT

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Fehér Zsolt]

Created attachment 116014 [details]
Patch

Would so good this?

[Csaba Osztrogonác]

Comment on attachment 116014 [details]
Patch

If m_expectedHash.isEmpty() is true then DRT won't dump the image.
So how can we generate pixel results for new tests after this change?

[Nandor Huszka]

If there is no objection, I would like to continue fixing the bug.

[Nandor Huszka]

Reproducing the bug:

1 - doing a missing test
mv LayoutTests/svg/custom/text-zoom-expected.png LayoutTests/svg/custom/text-zoom-renamed.png

2 - gives missing result, uses WebkitTestRunner
Tools/Scripts/new-run-webkit-tests --qt -p --no-new-test-results -2 svg/custom/text-zoom.xhtml

3 - the test won't fail, uses WebkitTestRunner
Tools/Scripts/new-run-webkit-tests --qt -p --no-new-test-results --skip-pixel-test-if-no-baseline -2 svg/custom/text-zoom.xhtml


The goal is for

4 - uses DumpRenderTree
Tools/Scripts/new-run-webkit-tests --qt -p --no-new-test-results --skip-pixel-test-if-no-baseline svg/custom/text-zoom.xhtml

to not give fail.

[Nandor Huszka]

Created attachment 130591 [details]
Patch

[Dirk Pranke]

Comment on attachment 130591 [details]
Patch

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

Technology in new-run-webkit-tests has evolved over the past few months, and I think we should use a different approach for this patch. We now have the ability to run different tests with different command line flags (using the code in DriverProxy in webkitpy/layout_tests/port/driver.py), and given that we know whether we should run pixel tests or not in worker.py (as your change indicates), we should combine pass test_input.should_run_pixel_test to driver_input and pass run_pixel_test to driver_input and just use a different DRT instance to run the test in this case. 

If we do that, then you don't need to modify DRT at all and the logic of needing pixel tests or not stays where it should be, in worker.py and single_test_runner.py.

Let me know if that's enough for you to work on, or if you would like me to work up a patch to illustrate this.

Thanks!

> Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:126
> +            if (driver_name == "WebKitTestRunner" or driver_name == "DumpRenderTree") and self._port.get_option('skip_pixel_test_if_no_baseline') and self._port.get_option('pixel_tests'):

drivers are basically always either named 'WebKitTestRunner' or 'DumpRenderTree'. You can remove these checks and just check the port.get_option(X) values.

[Tony Chang]

(In reply to comment #5)
> (From update of attachment 130591 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130591&action=review
> 
> Technology in new-run-webkit-tests has evolved over the past few months, and I think we should use a different approach for this patch. We now have the ability to run different tests with different command line flags (using the code in DriverProxy in webkitpy/layout_tests/port/driver.py), and given that we know whether we should run pixel tests or not in worker.py (as your change indicates), we should combine pass test_input.should_run_pixel_test to driver_input and pass run_pixel_test to driver_input and just use a different DRT instance to run the test in this case. 

I thought we wanted to stop running multiple DRTs with different command lines (and remove the current multiple command line cases)?  For example, on Qt and GTK+, each new DRT will likely require a new Xvfb.

[Dirk Pranke]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 130591 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=130591&action=review
> > 
> > Technology in new-run-webkit-tests has evolved over the past few months, and I think we should use a different approach for this patch. We now have the ability to run different tests with different command line flags (using the code in DriverProxy in webkitpy/layout_tests/port/driver.py), and given that we know whether we should run pixel tests or not in worker.py (as your change indicates), we should combine pass test_input.should_run_pixel_test to driver_input and pass run_pixel_test to driver_input and just use a different DRT instance to run the test in this case. 
> 
> I thought we wanted to stop running multiple DRTs with different command lines (and remove the current multiple command line cases)?  For example, on Qt and GTK+, each new DRT will likely require a new Xvfb.

We do, but I want to make that change uniformly across all of the ports. Adding another command line flag like in this patch (which affects the behavior on all tests) is just going to complicate thing further.

[Nandor Huszka]

(In reply to comment #5)

> drivers are basically always either named 'WebKitTestRunner' or 'DumpRenderTree'. You can remove these checks and just check the port.get_option(X) values.

Ok, I see it.

> We now have the ability to run different tests with different command line flags (using the code in DriverProxy in webkitpy/layout_tests/port/driver.py), 

I have found it, it is DriverProxy's run_test method which is called by SingleTestRunner, but there are some thing that aren't clear for me.

> given that we know whether we should run pixel tests or not in worker.py (as your change indicates), we should combine pass test_input.should_run_pixel_test to driver_input and pass run_pixel_test to driver_input and just use a different DRT instance to run the test in this case. 

I can't found the run_pixel_test you have mentioned. Is it a method, or a member? I don't understand how should the combined passing mechanism work.

> If we do that, then you don't need to modify DRT at all and the logic of needing pixel tests or not stays where it should be, in worker.py and single_test_runner.py.

Should we remove all of the modifications from WebkitTestRunner that were made by https://bugs.webkit.org/show_bug.cgi?id=70484, and we don't need those ones in DRT that can be found in the patch above too? I mean we should handle it only with Python code?
If I understood it correctly, we should always skip pixeltest for a test if there isn't any expected png which belongs to it. So we won't use the command line argument --skip-pixel-test-if-no-baseline for evoking this behavior, will we?

It is difficult for me to comprehent, please descibe your request more detailed. Thank you!

[Dirk Pranke]

(In reply to comment #8)
> > given that we know whether we should run pixel tests or not in worker.py (as your change indicates), we should combine pass test_input.should_run_pixel_test to driver_input and pass run_pixel_test to driver_input and just use a different DRT instance to run the test in this case. 
> 
> I can't found the run_pixel_test you have mentioned. Is it a method, or a member? I don't understand how should the combined passing mechanism work.
>

I'm sorry, I could have been a little clearer. Change the is_reftest member of DriverInput to run_pixel_test and assign it the value we compute in worker and singletestrunner.
 
> > If we do that, then you don't need to modify DRT at all and the logic of needing pixel tests or not stays where it should be, in worker.py and single_test_runner.py.
> 
> Should we remove all of the modifications from WebkitTestRunner that were made by https://bugs.webkit.org/show_bug.cgi?id=70484, and we don't need those ones in DRT that can be found in the patch above too? I mean we should handle it only with Python code?

I probably would remove the previous changes in WebkitTestRunner, yes. I am saying we can handle it completely in Python code (for now; eventually we want to pass in whether we need pixel results or not as part of the per-test input, but that's a whole different discussion you should ignore for now :).

> If I understood it correctly, we should always skip pixeltest for a test if there isn't any expected png which belongs to it. So we won't use the command line argument --skip-pixel-test-if-no-baseline for evoking this behavior, will we?

Different webkit ports want different behavior. The Qt port wants to skip the pixel tests, but the Chromium port doesn't, for example. So we still need the flag on new-run-webkit-tests.

I hope this helps; let me know if you have further questions?

[Nandor Huszka]

Created attachment 131584 [details]
WIP patch

(In reply to comment #9)
> I hope this helps; let me know if you have further questions?
It helped, thank you. Is this similar to your notion?

[Nandor Huszka]

Created attachment 131598 [details]
WIP patch

Newer version of the patch, I have refactored it.

If we run the command

Tools/Scripts/new-run-webkit-tests --qt -p --no-new-test-results --skip-pixel-test-if-no-baseline svg/custom/text-zoom.xhtml

the pixel_tests_needed and self._run_pixel_test are False in DriverProxy' run_test method. I do not understand why cmd_line_key contains the --pixel-test option in DriverProxy/run_test in this case. Dirk, do you have any idea for this? Aside from, will it be approximately the proper way for handling tests that do not have pngs?

[Dirk Pranke]

Comment on attachment 131598 [details]
WIP patch

This looks pretty close to what I had in mind. If you make the changes I suggest below, that might fix your problems; if not, I'll probably have to take another look at it to figure out what's going on.

Thanks!

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

> Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:114
> +        # because it still does pixeltests in this case

this logic should be inside self._driver_input().

> Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:126
> +            if self._port.get_option('skip_pixel_test_if_no_baseline') and self._port.get_option('pixel_tests'):

I think the logic in this branch looks shady, but that's probably because we have the self._pixel_tests logic in other places as well. We should consolidate it all here, probably to something like:

if self._port.get_option('pixel_tests'):
    if self._port.get_option('skip_pixel_test_if_no_baseline'):
        test_input.should_run_pixel_test = (self._port.expected_image(test_input.test_name) != None)
    else:
        test_input.should_run_pixel_test = True
else:
    test_input.should_run_pixel_test = False

> Tools/Scripts/webkitpy/layout_tests/port/driver.py:211
> +        pixel_tests_needed = self._pixel_tests or driver_input.run_pixel_test

I think you can get rid of the self._pixel_tests member completely and just use driver_input.run_pixel_test here.

> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:559
> +            self._start(driver_input.run_pixel_test or self._pixel_tests, [])

See comment above and see if we can remove self._pixel_tests here.

[Nandor Huszka]

Created attachment 131828 [details]
Patch

(In reply to comment #12
I had to modify the WebKitDriver's cmd_line method too, because it passes the --pixel-test option to the drivers every time, but it should not. This was the cause of the problem I have mentioned in the previous comment.
Thank you for the information again, now it was detailed enough. :)

[Dirk Pranke]

Comment on attachment 116014 [details]
Patch

clearing r? on this patch since we're taking a different approach.

[Dirk Pranke]

Comment on attachment 131828 [details]
Patch

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

Almost there. Make sure that test-webkitpy passes after this next iteration and you should be all set.

> Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:97
> +            self._driver._pixel_tests = False

instead of modifying self._driver._pixel_tests here (which should be a private member of driver), just modify the driver implementations and delete the _pixel_tests member from them.

> Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py:213
> +        if test_input.run_pixel_test:

This won't actually work. This code needs to know if something is actually a reftest. You will need to modify the run(), input_from_line(), and output_for_test() methods to pass around is_reftest as a separate argument.

> Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py:276
> +        if self._options.pixel_tests and (test_input.image_hash or test_input.run_pixel_test):

See above. This needs to stay as is_reftest, as a separate argument.

> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:482
> +        if pixel_tests and self._pixel_tests:

Just remove the reference to self._pixel_tests completely.

[Nandor Huszka]

Created attachment 134036 [details]
Patch

(In reply to comment #15)
> just modify the driver implementations and delete the _pixel_tests member from them.

I have made the modifications that you suggested, and removed self._pixel_tests from DriverProxy. How should I modify the driver implementations? And in your opinion what cause the fail in it:

FAIL: test_crash_log (webkitpy.layout_tests.port.chromium_unittest.ChromiumDriverTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/hnandor/clean/Tools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py", line 110, in test_crash_log
    self.assertEqual(driver_output.crash_log, 'mockcrashlog')
AssertionError: <webkitpy.thirdparty.mock.Mock object at 0x3a8ed50> != 'mockcrashlog'

Only this one unittest fails and it seems --skip-pixel-test-if-no-baseline option works good.

[Dirk Pranke]

The py(In reply to comment #16)
> Created an attachment (id=134036) [details]
> Patch
> 
> (In reply to comment #15)
> > just modify the driver implementations and delete the _pixel_tests member from them.
> 
> I have made the modifications that you suggested, and removed self._pixel_tests from DriverProxy. How should I modify the driver implementations?

You should be able to remove any references to self._pixel_tests in driver.py, chromium.py, and webkit.py as well. 

> And in your opinion what cause the fail in it:
> 
> FAIL: test_crash_log (webkitpy.layout_tests.port.chromium_unittest.ChromiumDriverTest)

I would bet this is unrelated. Does it still happen if you sync to the current tip of the tree?

The patch looks good to me otherwise, but I'd like someone else to sanity check the changes to TestController .. ossy or tony or adam, can you do that?

[Nandor Huszka]

Created attachment 134238 [details]
Patch

(In reply to comment #17)
> I would bet this is unrelated. Does it still happen if you sync to the current tip of the tree?
Yes, this was the problem, now every unittest pass.

[Early Warning System Bot]

Comment on attachment 134238 [details]
Patch

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

[Gustavo Noronha (kov)]

Comment on attachment 134238 [details]
Patch

Attachment 134238 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12153040

[Build Bot]

Comment on attachment 134238 [details]
Patch

Attachment 134238 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12153043

[Nandor Huszka]

Created attachment 134242 [details]
Patch

(In reply to comment #21)
> (From update of attachment 134238 [details])
> Attachment 134238 [details] did not pass win-ews (win):
> Output: http://queues.webkit.org/results/12153043

Oops, I forgot removing a line from WebkitTestRunner and renaming an is_reftest reference. I hope now it will be good.

[Tony Chang]

Comment on attachment 134242 [details]
Patch

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

The changes to WTR seem fine. Just some style nits in the python code. I'll let Dirk give the final review.

> Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:99
> +        driver_input = DriverInput(self._test_name, self._timeout, image_hash, bool(self._reference_files))
> +        if not self._should_run_pixel_test:
> +            driver_input.run_pixel_test = False
> +        else:
> +            driver_input.run_pixel_test = True

Can we pass "not self._should_run_pixel_test" as the 4th argument to the DriverInput constructor?  It looks like we're just overwriting the value after it's set.

> Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:149
> +            if self._port.get_option('pixel_tests'):
> +                if self._port.get_option('skip_pixel_test_if_no_baseline'):
> +                    test_input.should_run_pixel_test = (self._port.expected_image(test_input.test_name) != None)
> +                else:
> +                    test_input.should_run_pixel_test = True
> +            else:
> +                test_input.should_run_pixel_test = False

This could be:

if self._port.get_option('pixel_tests') and self._port.get_option('skip_pixel_test_if_no_baseline'):
    test_input.should_run_pixel_test = (self._port.expected_image(test_input.test_name) != None)
else:
     test_input.should_run_pixel_test = self._port.get_option('pixel_tests')

> Tools/Scripts/webkitpy/layout_tests/port/driver.py:41
> -        self.is_reftest = is_reftest
> +        self.run_pixel_test = run_pixel_test

Nit: I like should_run_pixel_test (sounds like a bool) better than run_pixel_test (sounds like an action or function).

> Tools/Scripts/webkitpy/layout_tests/port/driver.py:227
> -        self._driver.start(self._pixel_tests, [])
> +        self._driver.start(True, [])

It's not obvious to me why we pass True here? Does this mean perftestrunner will have pixel dumping enabled (is that the old behavior)? Maybe it's OK because DriverProxy.start will be removed soon?

> Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py:207
>          return DriverInput(test_name, 0, checksum, is_reftest)

Nit: It might be more readable to make the 4th argument more explicit.  E.g., run_pixel_test=is_reftest.

> Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py:272
>          return DriverInput(test_name, timeout, checksum, is_reftest)

Nit: It might be more readable to make the 4th argument more explicit.  E.g., run_pixel_test=is_reftest.

[Dirk Pranke]

Comment on attachment 134242 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:149
>> +                test_input.should_run_pixel_test = False
> 
> This could be:
> 
> if self._port.get_option('pixel_tests') and self._port.get_option('skip_pixel_test_if_no_baseline'):
>     test_input.should_run_pixel_test = (self._port.expected_image(test_input.test_name) != None)
> else:
>      test_input.should_run_pixel_test = self._port.get_option('pixel_tests')

I'm not sure that this is clearer; the logic is kinda convoluted either way.

>> Tools/Scripts/webkitpy/layout_tests/port/driver.py:41
>> +        self.run_pixel_test = run_pixel_test
> 
> Nit: I like should_run_pixel_test (sounds like a bool) better than run_pixel_test (sounds like an action or function).

works for me.

>> Tools/Scripts/webkitpy/layout_tests/port/driver.py:227
>> +        self._driver.start(True, [])
> 
> It's not obvious to me why we pass True here? Does this mean perftestrunner will have pixel dumping enabled (is that the old behavior)? Maybe it's OK because DriverProxy.start will be removed soon?

The value is overridden in practice by the run_test() call, but picking the right default might save us from starting a non-pixel-test DRT and then having to start a second one down the road (or vice versa). This should probably either be self._port.get_option('pixel_tests') or stay as self._pixel_tests (and then keep line 184 as well.

Tony's other nits seem fine to me. I'll mark this R+, CQ-; you can upload another patch w/ the nits addressed and then anyone should be able to CQ+ it.

[Nandor Huszka]

Created attachment 134530 [details]
Patch

Tony, Dirk, thank you for the comments, I fixed the lines you have suggested.

[Csaba Osztrogonác]

Comment on attachment 134530 [details]
Patch

It conflicts with ToT now. :( Could you update it, please?

[Nandor Huszka]

Created attachment 136656 [details]
Patch

(In reply to comment #26)
> (From update of attachment 134530 [details])
> It conflicts with ToT now. :( Could you update it, please?

Of course, I have updated, and uploaded it again.

[Dirk Pranke]

Comment on attachment 136656 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py:188
> +            is_reftest = driver_input.should_run_pixel_test

I think this is probably wrong, and that you need to use the logic on lines 205-206 to set is_reftest, and then probably (I haven't tried this) set should_run_pixel_tests to (self._options.pixel_tests or driver_input.image_hash).

I don't think I have all the unit tests I need yet to make sure mock_drt doesn't break, but I'd be curious to know if mock_drt_unittest.py actually passes with this change? You should also do something like 'run-webkit-tests --platform mock-mac-lion' and 'run-webkit-tests --platform mock-gtk' and make sure the script runs to completion without any crashes or anything else weird happening.

Note that if you can actually run these steps with this patch as-is and everything looks right, then it's possible that I'm wrong and your patch is right :).

The mock_drt logic is kind of subtle  and confusing, and I'm probably the only one who's ever really used it; if it's not clear what the right changes are to get things to work, let me know and I'll see if I can update the patch correctly.

(And note that until very recently, all of this code was just broken anyway, which is why you weren't getting the feedback until now).

[Nandor Huszka]

(In reply to comment #28)
> (From update of attachment 136656 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136656&action=review 
> I don't think I have all the unit tests I need yet to make sure mock_drt doesn't break, but I'd be curious to know if mock_drt_unittest.py actually passes with this change? You should also do something like 'run-webkit-tests --platform mock-mac-lion' and 'run-webkit-tests --platform mock-gtk' and make sure the script runs to completion without any crashes or anything else weird happening.

You are right, these commands fail with this error:
File "/home/hnandor/WebKit/Tools/Scripts/webkitpy/layout_tests/port/webkit.py", line 545, in run_test
    self._driver.start(self._port.get_option('pixel_tests'), [])
AttributeError: 'GtkDriver' object has no attribute '_driver'

But I do not understand why the system is missing the _driver member. I did not removed it, is this because of this
> 
> > Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py:188
> > +            is_reftest = driver_input.should_run_pixel_test
> 

modification in your opinion? I do not have any idea for it, what could we do?

[Dirk Pranke]

(In reply to comment #29)
> You are right, these commands fail with this error:
> File "/home/hnandor/WebKit/Tools/Scripts/webkitpy/layout_tests/port/webkit.py", line 545, in run_test
>     self._driver.start(self._port.get_option('pixel_tests'), [])
> AttributeError: 'GtkDriver' object has no attribute '_driver'
> 
> But I do not understand why the system is missing the _driver member. I did not removed it, is this because of this

That object is a driver (a GtkDriver). You changed that line from self._start() to self._driver.start() (probably a cut&paste from DriverProxy in driver.py). Change it back :).

-- Dirk

[Nandor Huszka]

Created attachment 137313 [details]
Patch

> That object is a driver (a GtkDriver). You changed that line from self._start() to self._driver.start() (probably a cut&paste from DriverProxy in driver.py). Change it back :).
Thank you, it was a carelessness on my part.

> I think this is probably wrong, and that you need to use the logic on lines 205-206 to set is_reftest, and then probably (I haven't tried this) set should_run_pixel_tests to (self._options.pixel_tests or driver_input.image_hash).
I have run unittests with
    is_reftest = driver_input.should_run_pixel_test
    driver_input.should_run_pixel_test = (self._options.pixel_tests or driver_input.image_hash)
modifications in mock_drt.py:188, but 16 fail appeared, they were similar to this one:

ERROR: webkitpy.layout_tests.port.mock_drt_unittest.MockDRTTest.test_textonly
--------------------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/hnandor/WebKit/Tools/Scripts/webkitpy/layout_tests/port/mock_drt_unittest.py", line 192, in test_textonly
    self.assertTest('passes/image.html', False)
  File "/home/hnandor/WebKit/Tools/Scripts/webkitpy/layout_tests/port/mock_drt_unittest.py", line 154, in assertTest
    res = drt.run()
  File "/home/hnandor/WebKit/Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py", line 188, in run
    is_reftest = (self._port.reference_files(test_name) or
NameError: global name 'test_name' is not defined

Then I tried this:
    is_reftest = driver_input.should_run_pixel_test
    driver_input.should_run_pixel_test = (self._options.pixel_tests or driver_input.image_hash)
Only one unittest failed with it:

FAILURE: webkitpy.layout_tests.port.mock_drt_unittest.MockDRTTest.test_missing_image
--------------------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/hnandor/WebKit/Tools/Scripts/webkitpy/layout_tests/port/mock_drt_unittest.py", line 198, in test_missing_image
    self.assertTest('failures/expected/missing_image.html', True)
  File "/home/hnandor/WebKit/Tools/Scripts/webkitpy/layout_tests/port/mock_drt_unittest.py", line 160, in assertTest
    self.assertEqual(stdout.buflist, drt_output)
AssertionError: Lists differ: ['Content-Type: text/plain\n',... != ['Content-Type: text/plain\n',...

First differing element 3:

#EOF

First list contains 3 additional elements.
First extra element 4:
ActualHash: None

+ ['Content-Type: text/plain\n', 'missing_image-txt', '#EOF\n', '#EOF\n']
- ['Content-Type: text/plain\n',
-  'missing_image-txt',
-  '#EOF\n',
-  '\n',
-  'ActualHash: None\n',
-  'ExpectedHash: None\n',
-  '#EOF\n']

And with this combination:
    driver_input.should_run_pixel_test = (self._options.pixel_tests or driver_input.image_hash)
    is_reftest = driver_input.should_run_pixel_tests
11 fails were generated.

But with only this line:
    is_reftest = driver_input.should_run_pixel_test
all of the unittests passed. It means that there is no need for the logic on lines 205-206 here?
You were right, I have not run unittests before uploading the last patch and some other unittests failed because of wrong parameter number (output_for_test) and missing renaming (test_input.is_reftest) in mock_drt.py.
Now the run-webkit-tests --platform mock-gtk, run-webkit-tests --platform mock-mac-lion and testwebkitpy run without any errors, and I hope it means everything is OK with this version of patch. :)

[Dirk Pranke]

Argh, this logic is convoluted. I've applied your patch locally and it doesn't do the right thing, and now it's not clear what the right thing is :) ... I'm working on it and will report back.

[Eric Seidel (no email)]

Comment on attachment 134242 [details]
Patch

Cleared Dirk Pranke's review+ from obsolete attachment 134242 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Dirk Pranke]

Created attachment 138019 [details]
Patch

[Dirk Pranke]

Okay, it turns out that your patch wasn't properly accounting for whether a test was a reftest or not in worker.py and single_test_runner.py: we always want to tell DRT/WTR to run a pixel test if the test is a reftest.

So, I've uploaded a new, revised patch that makes things work (as far as I can tell) and cleans things up as well.

I moved the logic for figuring out if a test is reftest or not from single_test_runner to worker so that we could set should_run_pixel_test correctly.

That allowed me to see that MockDRT should *only* rely on the --pixel-tests parameter to figure out whether or not to look for and return image data - since reference tests should always have this flag be true - and thus split out whether or not to write pixel data (yes if --pixel-tests is on the command line) from *what* data to write (varies depending on if the test is a ref test or not).

Sheesh. You wouldn't have thought this change would be so hard to get right, but I think the code is cleaner as a result.

Let me know if this looks good to you, and I'll land it. Thanks for hanging in there!

[Nandor Huszka]

(In reply to comment #35)
> Okay, it turns out that your patch wasn't properly accounting for whether a test was a reftest or not in worker.py and single_test_runner.py: we always want to tell DRT/WTR to run a pixel test if the test is a reftest.

Oh, good to know this, now I am aware of it. 

> So, I've uploaded a new, revised patch that makes things work (as far as I can tell) and cleans things up as well.
> 
> I moved the logic for figuring out if a test is reftest or not from single_test_runner to worker so that we could set should_run_pixel_test correctly.
> 
> Sheesh. You wouldn't have thought this change would be so hard to get right, but I think the code is cleaner as a result.

This is my opinion too, now it is more comprehensive.

> Let me know if this looks good to you, and I'll land it. Thanks for hanging in there!

I have applied the patch and there is no trouble with it. Thank you for speeding up the process, and uploading this patch. It would take much more time if you had not done this. :)

[Dirk Pranke]

Committed r114788: <http://trac.webkit.org/changeset/114788>