Bug 71574 – [NRWT] result.html should not rely on naming convention of reference file used in reftests.

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Hayato Ito]

A result.html assumes that reftests have reference filename named '-expected.html' (or '-expected-mismatch.html').
But that won't apply for w3c reftests. We should make a result.html link to the correct reference file.

[Hayato Ito]

Created attachment 114250 [details]
use ref_file in results.html

[Adam Barth]

Comment on attachment 114250 [details]
use ref_file in results.html

All functional changes to webkitpy require tests.

[Adam Barth]

Even if this doesn't affect any LayoutTests, you can wrote webkitpy unit tests to exercise the change.

[Ryosuke Niwa]

(In reply to comment #3)
> Even if this doesn't affect any LayoutTests, you can wrote webkitpy unit tests to exercise the change.

I don't think we can test this change by a unit test in practice. Most of tests for manager.py are integration tests but we can't write an integration test until https://bugs.webkit.org/show_bug.cgi?id=66837 is fixed.

[Adam Barth]

Perhaps we need to wait until Bug 66837 is fixed before landing this patch.  Another option is to improve the testability of this code.

[Ojan Vafai]

It looks to me that this can now be tested using regular reftests since bug 71567 was committed.

[Ryosuke Niwa]

(In reply to comment #5)
> Perhaps we need to wait until Bug 66837 is fixed before landing this patch.  Another option is to improve the testability of this code.

Right. We need to wait 'til the bug 66837 is resolved.

[Ojan Vafai]

(In reply to comment #7)
> (In reply to comment #5)
> > Perhaps we need to wait until Bug 66837 is fixed before landing this patch.  Another option is to improve the testability of this code.
> 
> Right. We need to wait 'til the bug 66837 is resolved.

I don't understand why. WIth bug 71567 resolved, we can hit all these codepaths using the existing reftest infrastructure (e.g. with a -expected.html or -expected-mismatch.html file).

[Hayato Ito]

Created attachment 114429 [details]
add tests.

[Hayato Ito]

Hi Ojan,
Thank you for the comments.

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > Perhaps we need to wait until Bug 66837 is fixed before landing this patch.  Another option is to improve the testability of this code.
> > 
> > Right. We need to wait 'til the bug 66837 is resolved.
> 
> I don't understand why. WIth bug 71567 resolved, we can hit all these codepaths using the existing reftest infrastructure (e.g. with a -expected.html or -expected-mismatch.html file).


For existing reftests, we can hit all codepths.
But we cannot hit new codepath introduced in this patch, which requires a reftest which uses a non-default reference filename.

Therefore, I've refactored the previous patch so that we can test new codepath. I also added tests for results.html into results-test.js.

I was impressed that results.html has been also tested. :)

[Ryosuke Niwa]

Comment on attachment 114429 [details]
add tests.

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

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:71
> +def interpret_test_failures(port_obj, test_name, failures):

It's better to call it "port"

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:88
> +    for f in failures:

Please don't use one-letter variable.

> LayoutTests/fast/harness/results.html:552
> +            if (testObject.ref_file)
> +                row += referenceLink(test_prefix, testObject.ref_file, 'ref mismatch html');
> +            else
> +                row += resultLink(test_prefix, '-expected-mismatch.html', 'ref mismatch html');

Can we always add ref_file property instead?

[Hayato Ito]

Created attachment 114437 [details]
fixed variable name.

[Hayato Ito]

Created attachment 114439 [details]
fixed variable name, port

[Hayato Ito]

Thank you for the review.

(In reply to comment #11)
> (From update of attachment 114429 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114429&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:71
> > +def interpret_test_failures(port_obj, test_name, failures):
> 
> It's better to call it "port"

Done.

> 
> > Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:88
> > +    for f in failures:
> 
> Please don't use one-letter variable.

Done.

> 
> > LayoutTests/fast/harness/results.html:552
> > +            if (testObject.ref_file)
> > +                row += referenceLink(test_prefix, testObject.ref_file, 'ref mismatch html');
> > +            else
> > +                row += resultLink(test_prefix, '-expected-mismatch.html', 'ref mismatch html');
> 
> Can we always add ref_file property instead?

That's intentional. We can reduce the size of the full_results.json if we omit ref_file which matches the naming convention. I think it is worth doing.

[Ryosuke Niwa]

Comment on attachment 114439 [details]
fixed variable name, port

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

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:94
> +            if failure.reference_filename != port.reftest_expected_filename(test_name):
> +                test_dict['ref_file'] = port.relative_test_filename(failure.reference_filename)

I still think we can always add ref_file but that might bloat the json?

> LayoutTests/fast/harness/results.html:415
> +function referenceLink(testPrefix, reference_filename, contents)
> +{
> +    return '<a class=result-link href="' + reference_filename + '" data-prefix="' + testPrefix + '">' + contents + '</a> ';
> +}

Maybe we can share the code with resultLink?

[Hayato Ito]

Thank you for the review.
I'll land this patch after addressing your comment.

(In reply to comment #15)
> (From update of attachment 114439 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114439&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:94
> > +            if failure.reference_filename != port.reftest_expected_filename(test_name):
> > +                test_dict['ref_file'] = port.relative_test_filename(failure.reference_filename)
> 
> I still think we can always add ref_file but that might bloat the json?

Maybe I am too conservative, but I can find there are already a lot of efforts to reduce the size of full_results.json. So it might be safe to follow the local rule.

> 
> > LayoutTests/fast/harness/results.html:415
> > +function referenceLink(testPrefix, reference_filename, contents)
> > +{
> > +    return '<a class=result-link href="' + reference_filename + '" data-prefix="' + testPrefix + '">' + contents + '</a> ';
> > +}
> 
> Maybe we can share the code with resultLink?

Okay. That's trivial.

[Hayato Ito]

Committed r99818: <http://trac.webkit.org/changeset/99818>