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.
Created attachment 194223 [details] Work in progress
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.
Created attachment 194456 [details] Patch
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.
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.
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.
(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.
Committed r146657: <http://trac.webkit.org/changeset/146657>
(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?
(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.