Bug 66837 - Parse reftest.list and extract types of ref tests
Summary: Parse reftest.list and extract types of ref tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on: 66838
Blocks: 71574
  Show dependency treegraph
 
Reported: 2011-08-23 22:26 PDT by Hayato Ito
Modified: 2011-12-01 17:46 PST (History)
10 users (show)

See Also:


Attachments
work in progress (3.85 KB, patch)
2011-11-03 17:28 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
work in progress 2 (4.04 KB, patch)
2011-11-03 17:40 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch (5.83 KB, patch)
2011-11-04 08:46 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Moved the logic to find reftest.list to test_files (13.26 KB, patch)
2011-11-04 11:29 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed a bug in test_files (14.41 KB, patch)
2011-11-04 19:08 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch (20.78 KB, patch)
2011-11-17 18:10 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch (17.53 KB, patch)
2011-11-30 20:56 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Addressed the comments and updated for ToT (20.69 KB, patch)
2011-12-01 17:22 PST, Ryosuke Niwa
dpranke: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hayato Ito 2011-08-23 22:26:10 PDT
To run CSSWG reftests, we need to extract necessary information, such as reference links, from the Retest Test file.
http://wiki.csswg.org/test/css2.1/format#reference-links

Until we encounter a serious problem in parsing the HTML file, I think HTMLParser built in Python is enough to do these kinds of tasks.
Comment 1 Ryosuke Niwa 2011-11-03 17:28:41 PDT
Created attachment 113590 [details]
work in progress
Comment 2 Ryosuke Niwa 2011-11-03 17:40:15 PDT
Created attachment 113595 [details]
work in progress 2
Comment 3 Ryosuke Niwa 2011-11-04 08:46:42 PDT
Created attachment 113661 [details]
Patch
Comment 4 Ryosuke Niwa 2011-11-04 09:42:41 PDT
I didn't include a test for _parse_all_reftest_list because that involves mocking a whole bunch of objects and is a bit cumbersome. We can test it easily once hayato's change is in.
Comment 5 Ojan Vafai 2011-11-04 10:15:16 PDT
Comment on attachment 113661 [details]
Patch

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

You should parse the reftest.list files during the initial walk of the LayoutTests directory. That will reduce a lot of processing and minimize disk access. I don't think it will make the code more complicated.

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:340
> +        self._reftest_list = {}

This isn't a list. Just call it self._reftests.

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:361
> +            expectation_type, test_file, ref_file = split_line[0], split_line[1], split_line[2]

I believe this can just be:
expectation_type, test_file, ref_file = split_line
Comment 6 Ryosuke Niwa 2011-11-04 11:29:31 PDT
Created attachment 113686 [details]
Moved the logic to find reftest.list to test_files
Comment 7 Ryosuke Niwa 2011-11-04 19:08:02 PDT
Created attachment 113744 [details]
Fixed a bug in test_files
Comment 8 Ryosuke Niwa 2011-11-07 18:43:36 PST
Any reviewer?
Comment 9 Ojan Vafai 2011-11-07 18:48:04 PST
Comment on attachment 113744 [details]
Fixed a bug in test_files

As I said on webkit-dev, I'm not OK with generically adding manifest support. If we're going to support reftest lists, they should be used exclusively for test suites we import. So, I'd like to see this patch assume we'll import these test suites to a certain directory (e.g. w3c) and only walk that subtree and if that subtree has a reftest.list file, then it doesn't do the other walking of that directory.
Comment 10 Ojan Vafai 2011-11-07 18:52:53 PST
I don't think we should add manifest support until we hit a case where we *really* need it and have no other (performant) way of doing it. Until then, we should just support link elements.

If you really want to do the manifest approach, I recommend that you get other webkit reviewers to support doing it this way.
Comment 11 Ojan Vafai 2011-11-07 23:49:02 PST
Just to be clear, I think our initial implementation of reading the link elements can just use the python HTML parser. It may be slow, but we can have a FIXME until we actually encounter the situation where it's a meaningful percentage of the total test runtime, at which point we can move the logic into DRT. Until then, we should just do the expedient thing.
Comment 12 Ryosuke Niwa 2011-11-08 21:10:47 PST
I've asked Ossy and he told me he's going to talk with other Qt folks and get back to me tomorrow.
Comment 13 Ojan Vafai 2011-11-09 09:49:54 PST
Are you not OK with limiting the reftest support to the w3c directory and having reftest.list mean that we don't walk that subtree looking for other tests?

If you're OK with those limitations, then you can modify the patch and we can move on.
Comment 14 Ryosuke Niwa 2011-11-09 13:01:30 PST
(In reply to comment #13)
> Are you not OK with limiting the reftest support to the w3c directory and having reftest.list mean that we don't walk that subtree looking for other tests?
> 
> If you're OK with those limitations, then you can modify the patch and we can move on.

I'm still waiting for Ossy's response. I couldn't reach him today. Regardless, I'm probably not going to work on this bug for the rest of week so anyone should feel free to take over this bug.
Comment 15 Ryosuke Niwa 2011-11-10 11:57:41 PST
kling says he's going to talk with other Qt folks tomorrow morning.
Comment 16 Ryosuke Niwa 2011-11-17 12:22:32 PST
I talked with Ossy and he told me he's okay with using manifest files for imported tests. So let's do that for now.
Comment 17 Ryosuke Niwa 2011-11-17 18:10:12 PST
Created attachment 115719 [details]
Patch
Comment 18 Ryosuke Niwa 2011-11-18 12:05:52 PST
Ping reviewers.
Comment 19 Eric Seidel (no email) 2011-11-30 11:32:22 PST
Comment on attachment 115719 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:342
> +    def _parse_all_reftest_lists(self):

Why wouldn't this move onto a separate RefTestListParser object?  (Or something better named).  Why should the manager have this functionality?

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:366
> +    def _parse_reftest_list(self, reftest_list, test_dir):

This name doesnt' convey the fact that it's going to store the results in some variable on self.  I would expect it to either return something, or be named differently.
Comment 20 Dirk Pranke 2011-11-30 14:01:58 PST
Comment on attachment 115719 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:775
> +        self.assertEquals(res, 8)

Perhaps just change this to unexpected_tests_count and get rid of the comment?

>> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:342
>> +    def _parse_all_reftest_lists(self):
> 
> Why wouldn't this move onto a separate RefTestListParser object?  (Or something better named).  Why should the manager have this functionality?

It's not clear to me that this shouldn't be implemented inside of port.tests(), instead. In particular, port.reftest_expected_filename() and port.reftest_expected_mismatch_filename() won't work correctly with these reftests. We should have one uniform way of handling reftests, regardless of how they are implemented. This suggests that we probably need a port.is_reftest() and to hang the _reftests dict underneath port.

Arguably all of that stuff should be split into a separate object (along the lines of Eric's suggestion), but I want to keep all of the test-handling logic together, both for reftests and other kinds of tests.

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:609
> +        return TestInput(test_file, self._options.time_out_ms, ref_file, is_mismatch)

same thoughts as above.

> Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:72
> +            if fs.exists(reftest_expected_filename):

If we make the changes I describe, do we need should_use_naming_convention_for_reftest() or is the old code still more-or-less correct?
Comment 21 Ryosuke Niwa 2011-11-30 20:56:24 PST
Created attachment 117322 [details]
Patch
Comment 22 Dirk Pranke 2011-12-01 14:18:45 PST
Comment on attachment 117322 [details]
Patch

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

Patch generally looks fine, just a few nits and one question that I'm not sure about the answer to but I'm not sure that it should block the patch ...

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:454
> +        self.assertTrue(json_string.find('"num_flaky":0') != -1)

any particular reason these two lines changed, or was it just to make things more consistent?

> Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:76
> +        if reftest_expected_mismatch_filename and fs.exists(reftest_expected_mismatch_filename):

Given that we can't rely on files being named '-expected.html' any more, the error message on line 78+ is slightly misleading. Can you rewrite it to something like "One test file cannot have both match and mismatch references. Please remove either %s or %s" ?

> Tools/Scripts/webkitpy/layout_tests/port/base.py:151
> +        self._reftest_list = dict()

Style nit: I'd usually use {} instead of dict() here, but it doesn't really matter.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:449
> +            return None

Should this be either expected_filename(test_name, '.html') or expected_filename(test_name, '-mismatch.html') as well for compatibility with the branch in  lines 439-443? I.e., maybe this function should never return None? I'd have to look at the calling code to see if this actually matters.

> Tools/Scripts/webkitpy/layout_tests/port/test_files_unittest.py:87
> +== test-3.html test-ref.html"""

Style nit: I think these days we usually try to avoid HEREDOC-style multiline strings and instead use string concatenation, so that the indentation is preserved.
Comment 23 Ryosuke Niwa 2011-12-01 14:25:37 PST
Comment on attachment 117322 [details]
Patch

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

Thanks for the review. I'm thrilled to get over with this bike-shedding bug.

>> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:454
>> +        self.assertTrue(json_string.find('"num_flaky":0') != -1)
> 
> any particular reason these two lines changed, or was it just to make things more consistent?

They had to be changed because I've added new test files.

>> Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:76
>> +        if reftest_expected_mismatch_filename and fs.exists(reftest_expected_mismatch_filename):
> 
> Given that we can't rely on files being named '-expected.html' any more, the error message on line 78+ is slightly misleading. Can you rewrite it to something like "One test file cannot have both match and mismatch references. Please remove either %s or %s" ?

Will do.

Btw, that error message is flawed in the sense W3C reftests allow one test to have multiple reference files to compare to. So we need to fix nrwt.

>> Tools/Scripts/webkitpy/layout_tests/port/base.py:151
>> +        self._reftest_list = dict()
> 
> Style nit: I'd usually use {} instead of dict() here, but it doesn't really matter.

Okay. I wasn't sure about the convention. Will change (since I prefer {} as well).

>> Tools/Scripts/webkitpy/layout_tests/port/base.py:449

> 
> Should this be either expected_filename(test_name, '.html') or expected_filename(test_name, '-mismatch.html') as well for compatibility with the branch in  lines 439-443? I.e., maybe this function should never return None? I'd have to look at the calling code to see if this actually matters.

No, this has to return None. If there's a reftest.list, we shouldn't be using the naming convention to find a reference file. This was an explicit request from Ojan based on which he r-ed my previous patch.

>> Tools/Scripts/webkitpy/layout_tests/port/test_files_unittest.py:87

> 
> Style nit: I think these days we usually try to avoid HEREDOC-style multiline strings and instead use string concatenation, so that the indentation is preserved.

Will fix.
Comment 24 Dirk Pranke 2011-12-01 14:34:26 PST
(In reply to comment #23)
> > Given that we can't rely on files being named '-expected.html' any more, the error message on line 78+ is slightly misleading. Can you rewrite it to something like "One test file cannot have both match and mismatch references. Please remove either %s or %s" ?
> 
> Will do.
> 
> Btw, that error message is flawed in the sense W3C reftests allow one test to have multiple reference files to compare to. So we need to fix nrwt.
>

Good point. 
 
> > 
> > Should this be either expected_filename(test_name, '.html') or expected_filename(test_name, '-mismatch.html') as well for compatibility with the branch in  lines 439-443? I.e., maybe this function should never return None? I'd have to look at the calling code to see if this actually matters.
> 
> No, this has to return None. If there's a reftest.list, we shouldn't be using the naming convention to find a reference file. This was an explicit request from Ojan based on which he r-ed my previous patch.
>

I understand the concern; my point was rather that I'm not sure what'll happen to callers of reftest_expected_filename() and reftest_expected_mismatch() if they get back None instead of a (potentially non-existant) filename (since they always got filenames before). Returning None seems like it might be wrong way to signal an error here, and it might be better to either _log.error() or raise an exception ...

Ojan, WDYT?

-- Dirk
Comment 25 Ryosuke Niwa 2011-12-01 14:40:45 PST
(In reply to comment #24)
> > No, this has to return None. If there's a reftest.list, we shouldn't be using the naming convention to find a reference file. This was an explicit request from Ojan based on which he r-ed my previous patch.
> >
> 
> I understand the concern; my point was rather that I'm not sure what'll happen to callers of reftest_expected_filename() and reftest_expected_mismatch() if they get back None instead of a (potentially non-existant) filename (since they always got filenames before). Returning None seems like it might be wrong way to signal an error here, and it might be better to either _log.error() or raise an exception ...

I hit an exception in signle_test_runner so I fixed that.

Only other callers are dryrun.py and rebaseline_choromium_webkit_tests.py, both of which just calls fs.exits. I guess I'll add is_reftest(test_name) to the port object and replace those calls by that.
Comment 26 Ryosuke Niwa 2011-12-01 17:22:44 PST
Created attachment 117530 [details]
Addressed the comments and updated for ToT
Comment 27 Ryosuke Niwa 2011-12-01 17:46:27 PST
Committed r101727: <http://trac.webkit.org/changeset/101727>