Created attachment 113590 [details] work in progress

Created attachment 113595 [details] work in progress 2

Created attachment 113661 [details] Patch

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

Created attachment 113686 [details] Moved the logic to find reftest.list to test_files

Created attachment 113744 [details] Fixed a bug in test_files

Any reviewer?

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.

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.

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.

I've asked Ossy and he told me he's going to talk with other Qt folks and get back to me tomorrow.

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.

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

kling says he's going to talk with other Qt folks tomorrow morning.

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.

Created attachment 115719 [details] Patch

Ping reviewers.

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 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?

Created attachment 117322 [details] Patch

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

(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

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

Created attachment 117530 [details] Addressed the comments and updated for ToT

Committed r101727: <http://trac.webkit.org/changeset/101727>