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

Comment on attachment 114250 [details] use ref_file in results.html All functional changes to webkitpy require tests.

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

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

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

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

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

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

Created attachment 114429 [details] add tests.

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. :)

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?

Created attachment 114437 [details] fixed variable name.

Created attachment 114439 [details] fixed variable name, port

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.

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?

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.

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