Bug 70484

Created attachment 111744 [details] Patch

Created attachment 111789 [details] Patch

Comment on attachment 111744 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111744&action=review In general the concept is good but I'm not happy with the semantics of the new switch (wk2_pixel_test). > Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:100 > + if self._port.driver_name() == "WebKitTestRunner" and self._port.get_option('wk2_pixel_test'): > + return self._should_run_pixel_test 1. wk2_pixel_test does not express the concept. I would use a separate switch, something like --skip-pixel-test-if-no-baseline. It would be valid only with -p (alias --pixel-tests), so it should be ignored with a warning otherwise 2. it's ok to only support it with WTR, but in that case we should die with error message if it is used with DRT > Tools/WebKitTestRunner/TestInvocation.cpp:118 > +void TestInvocation::setIsWK2PixelTest(const std::string& expectedPixelHash) > +{ > + if (!expectedPixelHash.length()) > + return; > + m_dumpPixels = true; > + m_expectedPixelHash = expectedPixelHash; > +} According to 1, you could do it in setIsPixelTest. You should still have an extra bool if you want to check that there is no empty hash when running without the --skip... switch, but I don't thing it is really necessary.

Comment on attachment 111789 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111789&action=review > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:289 > + optparse.make_option("--wk2-pixel-test", action="store_true", > + dest="wk2_pixel_test", help="Enable pixel-to-pixel PNG comparisons on WebKit2"), Why not just reuse --pixel? You already know if you're webkit2 from the driver_name. Seems like you could simplify this patch a lot if you did so. If there's a reason you need a new flag, you should explain it in the changelog.

Created attachment 112558 [details] Diff

(In reply to comment #4) > (From update of attachment 111789 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=111789&action=review > > > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:289 > > + optparse.make_option("--wk2-pixel-test", action="store_true", > > + dest="wk2_pixel_test", help="Enable pixel-to-pixel PNG comparisons on WebKit2"), > > Why not just reuse --pixel? You already know if you're webkit2 from the driver_name. Seems like you could simplify this patch a lot if you did so. > > If there's a reason you need a new flag, you should explain it in the changelog. The new flag have a speaking name now. What do you think, that would be good?

Created attachment 112648 [details] Patch

Comment on attachment 112648 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112648&action=review If you don't set r? on the patch consider that it won't be reviewed! > Tools/ChangeLog:9 > + we will run pixel-tests in WebKit2, when the *-expected.png file is you don't need 'is', you need 'for' rather than 'to' => when the *-expected.png file exists for the test. > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:87 > + _log.error("--skip-pixel-test-if-no-baseline is only supperted in WebKitTestRunner") supported rather than supperted

Created attachment 112825 [details] Patch

Ping for review.

I cc-ed NRWT experts, Tony and Dirk. I think we should change the name of the bug, because the patch and the bug title is different. The proposed patch isn't enable pixel tests for Qt-WK2 platform, but make it possible to run pixel tests if there aren't checked in png results for all tests.

Comment on attachment 112825 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112825&action=review Can you write some python unit tests for this flag? > Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:113 > + if self._port.driver_name() == "WebKitTestRunner" and self._port.get_option('skip_pixel_test_if_no_baseline') and self._port.get_option('pixel_tests'): Can you use self._options.skip_pixel_test_if_no_baseline and self._options.pixel_tests instead? > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:86 > + if options.skip_pixel_test_if_no_baseline and not (port.driver_name() == "WebKitTestRunner"): Nit: != seems a little clearer than not (...)

Comment on attachment 112825 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112825&action=review >> Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:113 >> + if self._port.driver_name() == "WebKitTestRunner" and self._port.get_option('skip_pixel_test_if_no_baseline') and self._port.get_option('pixel_tests'): > > Can you use self._options.skip_pixel_test_if_no_baseline and self._options.pixel_tests instead? I think self._options is shared between this object and the enclosing worker object, meaning if you set this to False here, all subsequent tests run in that worker will also not use pixel tests. I'm guessing you don't want that. >> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:86 >> + if options.skip_pixel_test_if_no_baseline and not (port.driver_name() == "WebKitTestRunner"): > > Nit: != seems a little clearer than not (...) Why is this change specific to WebKitTestRunner ? Why wouldn't we want to support this in DRT as well?

Comment on attachment 112825 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112825&action=review >>> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:86 >>> + if options.skip_pixel_test_if_no_baseline and not (port.driver_name() == "WebKitTestRunner"): >> >> Nit: != seems a little clearer than not (...) > > Why is this change specific to WebKitTestRunner ? Why wouldn't we want to support this in DRT as well? Because we need now this patch on WK2, but an another patch we can supported in DRT also.

Comment on attachment 112825 [details] Patch I can make the fix, based on your comments and i write some unit test to this patch.

Created attachment 116019 [details] Patch The unittest, don't work for me, i think it is, becasue of a bug in nrwt. Any idea why don't work?

Attachment 116019 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/Scripts/webkitpy/layout_tests/contro..." exit_code: 1 Tools/Scripts/webkitpy/layout_tests/controllers/skip_pixel_test_if_no_baseline.py:25: expected 2 blank lines, found 1 [pep8/E302] [5] Tools/Scripts/webkitpy/layout_tests/models/test_input.py:55: no newline at end of file [pep8/W292] [5] Total errors found: 2 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 116019 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116019&action=review > Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:113 > + if skip_pixel_test_if_no_baseline.skip_pixel_test(self._port, test_input.test_name): Can't you just replace this with: test_input.should_run_pixel_test = (port.expected_image(test_name) == None) and delete skip_pixel_test_if_no_baseline.py ? Also, I still don't understand why you are limiting this flag to WebKitTestRunner. It doesn't seem like anything will break if you run this on other ports? > Tools/Scripts/webkitpy/layout_tests/port/webkit_unittest.py:159 > + if skip_pixel_test_if_no_baseline.skip_pixel_test(port, test_name): I suggest you just inline the function here as well. > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:69 > + return -1 As I suggested above, I don't think this check is necessary.

(In reply to comment #16) > Created an attachment (id=116019) [details] > Patch > > The unittest, don't work for me, i think it is, becasue of a bug in nrwt. > Any idea why don't work? Can you provide more information on why it isn't working? What is happening?

(In reply to comment #19) > (In reply to comment #16) > > Created an attachment (id=116019) [details] [details] > > Patch > > > > The unittest, don't work for me, i think it is, becasue of a bug in nrwt. > > Any idea why don't work? > > Can you provide more information on why it isn't working? What is happening? I think one of the method, which call this function work wrong. The unittest system throw this message: FAIL: test_skip_pixel_test_if_no_baseline_option (webkitpy.layout_tests.port.webkit_unittest.WebKitPortTest) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/feherzsolt/webkit/Tools/Scripts/webkitpy/layout_tests/port/webkit_unittest.py", line 162, in test_skip_pixel_test_if_no_baseline_option self.assertTrue(test_name in self.png_list) AssertionError: False is not true ---------------------------------------------------------------------- Ran 1237 tests in 22.667s FAILED (failures=1)

(In reply to comment #20) > (In reply to comment #19) > > (In reply to comment #16) > > > Created an attachment (id=116019) [details] [details] [details] > > > Patch > > > > > > The unittest, don't work for me, i think it is, becasue of a bug in nrwt. > > > Any idea why don't work? > > > > Can you provide more information on why it isn't working? What is happening? > > I think one of the method, which call this function work wrong. The unittest system throw this message: > > FAIL: test_skip_pixel_test_if_no_baseline_option (webkitpy.layout_tests.port.webkit_unittest.WebKitPortTest) > ---------------------------------------------------------------------- > Traceback (most recent call last): > File "/home/feherzsolt/webkit/Tools/Scripts/webkitpy/layout_tests/port/webkit_unittest.py", line 162, in test_skip_pixel_test_if_no_baseline_option > self.assertTrue(test_name in self.png_list) > AssertionError: False is not true > This is telling you that skip_pixel_test returned False, i.e., that one of the test files did have an expected image, but that that test wasn't in your png_list. You'll have to add some debug statements to figure out which test it is, and what port.expected_image() is returning (and why). Also, we try not to hard-code in real test names, and the TestWebKitPort is probably using a MockHost, which hopefully means a mock filesystem with a fake list of tests (set in webkitpy/layout_tests/port/test.py). If that is true, you should use some of the fake test names instead of the real test names you're using. > ---------------------------------------------------------------------- > Ran 1237 tests in 22.667s > > FAILED (failures=1)

(In reply to comment #18) > (From update of attachment 116019 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=116019&action=review > > > Also, I still don't understand why you are limiting this flag to WebKitTestRunner. It doesn't seem like anything will break if you run this on other ports? > Because we need now this patch on WK2, but an another patch we can supported in DRT also (In Bug: 72841(ID)).

Created attachment 117175 [details] Patch I uploaded the new patch with unittest and i made the fixes, what D. Pranke ask!

Comment on attachment 117175 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117175&action=review > Tools/Scripts/webkitpy/layout_tests/port/webkit_unittest.py:203 > + self.assertEquals(png_list, self.png_list) Okay, inlining the function made it clear that we weren't actually testing anything interesting by testing just that skip_pixel_test() returns the correct value. Further, writing tests in webkit_unittest that are so heavily dependent on test.py indicated that there was something wrong. I'd just delete this whole routine. What we need to test is that the actual change in worker.py is working correctly. The best way to do that is to is to add another test to run_webkit_tests_integrationtest.py with the correct flags. See something like test_repeat_each() for an example of how to check that we only ran the tests you expected to run. Of course, the integration tests all run using the TestPort, not the WebKitPort, and so they won't be able to check the restriction on this only working with WebKitTestRunner. We can either get rid of the generic restriction on this flag (in line 67 of run_webkit_test.py), or we can add a test-wk2 port implementation to port/test.py and override port.driver_name() accordingly. I would probably vote for just removing the check in run_webkit_tests.py; I think you're being overly cautious since the command line flag is kind of obscure, you're probably the only one to use it, and you're planning to add this to DRT as well, but if you would rather be safe than sorry, adding the test-wk2 flag is fine as well. Sorry for the back-and-forth on this :(. The patch looks fine otherwise.

(In reply to comment #24) > (From update of attachment 117175 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=117175&action=review > > > Tools/Scripts/webkitpy/layout_tests/port/webkit_unittest.py:203 > > + self.assertEquals(png_list, self.png_list) > > Okay, inlining the function made it clear that we weren't actually testing anything interesting by testing just that skip_pixel_test() returns the correct value. Further, writing tests in webkit_unittest that are so heavily dependent on test.py indicated that there was something wrong. I'd just delete this whole routine. > > What we need to test is that the actual change in worker.py is working correctly. The best way to do that is to is to add another test to run_webkit_tests_integrationtest.py with the correct flags. See something like test_repeat_each() for an example of how to check that we only ran the tests you expected to run. > > Of course, the integration tests all run using the TestPort, not the WebKitPort, and so they won't be able to check the restriction on this only working with WebKitTestRunner. We can either get rid of the generic restriction on this flag (in line 67 of run_webkit_test.py), or we can add a test-wk2 port implementation to port/test.py and override port.driver_name() accordingly. > > I would probably vote for just removing the check in run_webkit_tests.py; I think you're being overly cautious since the command line flag is kind of obscure, you're probably the only one to use it, and you're planning to add this to DRT as well, but if you would rather be safe than sorry, adding the test-wk2 flag is fine as well. > > Sorry for the back-and-forth on this :(. > > The patch looks fine otherwise. OK, i working on it now

Created attachment 123717 [details] Patch Sorry for the delay, i was on leave. I corrected my patch I hope, it meets now :)

Comment on attachment 123717 [details] Patch Attachment 123717 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11212813 New failing tests: media/audio-garbage-collect.html

Comment on attachment 123717 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123717&action=review > Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:112 > + if self._port.driver_name() == "WebKitTestRunner" and self._port.get_option('skip_pixel_test_if_no_baseline') and self._port.get_option('pixel_tests'): > + test_input.should_run_pixel_test = (self._port.expected_image(test_input.test_name) != None) I think a #FIXME comment telling that DRT should also support the option would be useful.

Created attachment 124486 [details] Patch

Created attachment 124508 [details] Patch

Err, I don't think we need one more iteration just because of the comment. I'm going to land the patch which has the r+ with the comment.

Comment on attachment 123717 [details] Patch Landed in http://trac.webkit.org/changeset/106222

Smg is definitely wrong with this: http://build.webkit.sed.hu/results/x86-32%20Linux%20Qt%20Release%20WebKit2/r106222%20(19365)/results.html. Ossy is going to rollout.

Comment on attachment 123717 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123717&action=review > Tools/Scripts/webkitpy/layout_tests/models/test_input.py:41 > - def __init__(self, test_name, timeout): > + def __init__(self, test_name, timeout, should_run_pixel_test=True): > """Holds the input parameters for a test. > Args: I think this is the reason why it broke the bot. This force pixel tests always, isn't it?

Rollout landed in http://trac.webkit.org/changeset/106224

And an other thing is wrong. Check http://build.webkit.sed.hu/results/x86-32%20Linux%20Qt%20Release%20WebKit2/r106222%20%2819365%29/results.html diff URL's don't work, because diff files aren't in the archive: http://build.webkit.sed.hu/builders/x86-32%20Linux%20Qt%20Release%20WebKit2/builds/19365/steps/archive-test-results/logs/stdio

(In reply to comment #34) > (From update of attachment 123717 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=123717&action=review > > > Tools/Scripts/webkitpy/layout_tests/models/test_input.py:41 > > - def __init__(self, test_name, timeout): > > + def __init__(self, test_name, timeout, should_run_pixel_test=True): > > """Holds the input parameters for a test. > > Args: > > I think this is the reason why it broke the bot. This force pixel tests always, isn't it? I don't think so, why i use this variable only, when the --skip-pixel-test-if-no-baseline option is enabled

Created attachment 126093 [details] Patch I did the repairs.

Comment on attachment 126093 [details] Patch Clearing flags on attachment: 126093 Committed r107113: <http://trac.webkit.org/changeset/107113>

All reviewed patches have been landed. Closing bug.