Bug 112898 – NRWT: Enable pixel tests when retrying tests

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Ryosuke Niwa]

It'll be helpful to regenerate actual results when we're retrying tests so that bots on build.webkit.org and EWS bots can generate actual results.

[Ryosuke Niwa]

Created attachment 194223 [details]
Work in progress

[Dirk Pranke]

Created attachment 194417 [details]
one-line patch to force pixel tests on on the retry

So, forcing pixel tests on on the retry should be a one-line change (as demonstrated in the patch I've just attached).

I haven't decided how I feel about this yet, because it's kind of a hack to change the options object, but it may be an acceptable hack.

There's at least a couple of other loose ends ...

as you noted on #webkit, it looks like the files aren't being written into retries/ on the retry. I'm not yet sure how that could be true, since I thought I had tests for this and at first glance the code looks right.

Also, printing out the unexpected results is a little strange; you get [ Failure Pass Failure ].

We might need to make results.html and/or results.json aware of which links (in which directories) to use as well.

[Ryosuke Niwa]

Created attachment 194456 [details]
Patch

[Dirk Pranke]

Comment on attachment 194456 [details]
Patch

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

Looks basically pretty good ... a few nits.

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:660
> +        res, err, _ = logging_run(['--no-pixel', 'failures/unexpected/text-image-checksum.html'], tests_included=True, host=host)

nit: I'd prefer --no-pixel-tests for this. --no-pixel is kinda of a deprecated hack.

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:-837
> -        self.assertEqual(len(stats['http']['tests']['passes']['image.html']['results']), 5)

you deleted this assert when you moved this to a free function. Do you need to add it back in to test_reftest_with_two_notrefs()?

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:202
> +                forced_pixel_tests_in_retry = self._force_pixel_tests_if_needed()

Nit: I'd probably call this "enable" rather than "force" ... it just sounds nicer :).

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:211
> +                    self._options.pixel_tests = False

Is this needed?

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

I waffled a bit on whether restarting the helper should be part of this routine or part of the caller, but I'm not sure if there's a clear win one way or another.

[Ryosuke Niwa]

Comment on attachment 194456 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:-837
>> -        self.assertEqual(len(stats['http']['tests']['passes']['image.html']['results']), 5)
> 
> you deleted this assert when you moved this to a free function. Do you need to add it back in to test_reftest_with_two_notrefs()?

Notice that these two statements appear after return statement so this was a dead code before the move.
Also, this assertion refers to some file in http/tests/passes, which isn't ran in test_reftest_with_two_notrefs.

I'm inclined to delete this dead code unless you have a suggestion as to how to fix it.

[Dirk Pranke]

Comment on attachment 194456 [details]
Patch

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

>>> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:-837
>>> -        self.assertEqual(len(stats['http']['tests']['passes']['image.html']['results']), 5)
>> 
>> you deleted this assert when you moved this to a free function. Do you need to add it back in to test_reftest_with_two_notrefs()?
> 
> Notice that these two statements appear after return statement so this was a dead code before the move.
> Also, this assertion refers to some file in http/tests/passes, which isn't ran in test_reftest_with_two_notrefs.
> 
> I'm inclined to delete this dead code unless you have a suggestion as to how to fix it.

Ah, you're right. Hm. ... yeah, deleting is fine, I guess.

[Ryosuke Niwa]

(In reply to comment #4)
> (From update of attachment 194456 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194456&action=review
>
> > Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:211
> > +                    self._options.pixel_tests = False
> 
> Is this needed?

Yes. Otherwise, results.html is going to think we've ran pixel tests as pixel_tests_enabled is set true in test_run_results.summarize_results.

[Ryosuke Niwa]

Committed r146657: <http://trac.webkit.org/changeset/146657>

[Csaba Osztrogonác]

(In reply to comment #8)
> Committed r146657: <http://trac.webkit.org/changeset/146657>

It caused a regression: https://bugs.webkit.org/show_bug.cgi?id=113223 
Could you check it, please?

[Ryosuke Niwa]

(In reply to comment #9)
> (In reply to comment #8)
> > Committed r146657: <http://trac.webkit.org/changeset/146657>
> 
> It caused a regression: https://bugs.webkit.org/show_bug.cgi?id=113223 
> Could you check it, please?

Yup, I just realized it. Sorry about that. Will try to check in a fix today.