This bug is an extension of the bug #97242. In results.html the unexpectedly-passing reftests are currently only reported when pixel testing was enabled during the test run.
Created attachment 165313 [details] Patch
Comment on attachment 165313 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165313&action=review The python change here makes it so that *every* test is treated as a reftest. Not sure off the top of my head what the correct code is, but we should only mark reftests as reftests. I'd prefer if we also keep is_reftest only for matching reftests. It just complicates the if-statement on the JS side to need to check both is_reftest and is_mismatch_reftest. Also, can you please write a test for the results.html change? > LayoutTests/fast/harness/results.html:515 > + if (expected != 'IMAGE' || globalState().results.pixel_tests_enabled || testObject.is_reftest) { This should be (expected != 'IMAGE' || (globalState().results.pixel_tests_enabled || testObject.is_reftest)), no?
(In reply to comment #2) > (From update of attachment 165313 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=165313&action=review > > The python change here makes it so that *every* test is treated as a reftest. Not sure off the top of my head what the correct code is, but we should only mark reftests as reftests. nm this. I misread the code.
(In reply to comment #2) > > I'd prefer if we also keep is_reftest only for matching reftests. It just complicates the if-statement on the JS side to need to check both is_reftest and is_mismatch_reftest. > > Also, can you please write a test for the results.html change? Sure. > > LayoutTests/fast/harness/results.html:515 > > + if (expected != 'IMAGE' || globalState().results.pixel_tests_enabled || testObject.is_reftest) { > > This should be (expected != 'IMAGE' || (globalState().results.pixel_tests_enabled || testObject.is_reftest)), no? Yeah, that works as well.
(In reply to comment #4) > (In reply to comment #2) > > > LayoutTests/fast/harness/results.html:515 > > > + if (expected != 'IMAGE' || globalState().results.pixel_tests_enabled || testObject.is_reftest) { > > > > This should be (expected != 'IMAGE' || (globalState().results.pixel_tests_enabled || testObject.is_reftest)), no? > > Yeah, that works as well. Sigh. I clearly was not awake when I reviewed this patch this morning. Ignore this as well.
Now that I'm actually awake...this patch looks good as is. Just needs a test.
(In reply to comment #6) > Now that I'm actually awake...this patch looks good as is. Just needs a test. Refactored it a bit in that time, I'll upload it anyway.
Created attachment 165408 [details] Patch
Comment on attachment 165408 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165408&action=review This is a nice cleanup. Would be great if you addressed my one comment, but I understand if you'd rather just add a FIXME and commit this. > Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:174 > + if '==' in result.reftest_type: > + test_dict['is_reftest'] = True > + if '!=' in result.reftest_type: > + test_dict['is_mismatch_reftest'] = True How about just putting reftest_type in test_dict directly and changing results.html to look at that? You can make this a FIXME if you don't want to do it now.
Created attachment 165421 [details] Patch Uploading a modified patch so you can confirm the changes look somewhat similar to what you had in mind.
Comment on attachment 165421 [details] Patch Looks great. Thanks. This patch exposes the fact that we handle multiple reference files completely incorrectly in results.html. We can deal with that problem when we actually have tests with multiple references in the tree.
Comment on attachment 165421 [details] Patch Clearing flags on attachment: 165421 Committed r129459: <http://trac.webkit.org/changeset/129459>
All reviewed patches have been landed. Closing bug.
One python test started failing after this landed: [245/1584] webkitpy.layout_tests.run_webkit_tests_integrationtest.EndToEndTest.test_reftest_with_two_notrefs failed: Traceback (most recent call last): File "/home/buildslave-1/webkit-buildslave/efl-linux-64-debug/build/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py", line 967, in test_reftest_with_two_notrefs {"expected": "PASS", "actual": "IMAGE", "image_diff_percent": 1, 'is_reftest': True}) AssertionError: {u'expected': u'PASS', u'actual': u'IMAGE', u'reftest_type': [u'=='], u'image_di [truncated]... != {'expected': 'PASS', 'is_reftest': True, 'actual': 'IMAGE', 'image_diff_percent' [truncated]... - {u'actual': u'IMAGE', ? - - + {'actual': 'IMAGE', - u'expected': u'PASS', ? - - + 'expected': 'PASS', - u'image_diff_percent': 1, ? - + 'image_diff_percent': 1, - u'reftest_type': [u'==']} + 'is_reftest': True} Ran 1584 tests in 2.911s FAILED (failures=1, errors=0)
(In reply to comment #14) > One python test started failing after this landed: Thanks for reporting, the test cases needed updating which I forgot to do when uploading the latest iteration of the patch. I fixed it in http://trac.webkit.org/changeset/129465.