Bug 97426 – Unexpected reftest passes are only reported when pixel testing is enabled in results.html as well

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Zan Dobersek]

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.

[Zan Dobersek]

Created attachment 165313 [details]
Patch

[Ojan Vafai]

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?

[Ojan Vafai]

(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.

[Zan Dobersek]

(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.

[Ojan Vafai]

(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.

[Ojan Vafai]

Now that I'm actually awake...this patch looks good as is. Just needs a test.

[Zan Dobersek]

(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.

[Zan Dobersek]

Created attachment 165408 [details]
Patch

[Ojan Vafai]

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.

[Zan Dobersek]

Created attachment 165421 [details]
Patch

Uploading a modified patch so you can confirm the changes look somewhat similar to what you had in mind.

[Ojan Vafai]

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.

[Zan Dobersek]

Comment on attachment 165421 [details]
Patch

Clearing flags on attachment: 165421

Committed r129459: <http://trac.webkit.org/changeset/129459>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

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)

[Zan Dobersek]

(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.