Bug 69750

Created attachment 110811 [details] draft patch It is working fine in local but do you have any idea in connection with this?

View in context: https://bugs.webkit.org/attachment.cgi?id=110811&action=review Some notes regarding the code: > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:347 > + index = 0; > + for line in expectations.split('\n'): you should use enumerate(expectations.split('\n')) that will give an additional index value also (http://docs.python.org/library/functions.html#enumerate), so you can ditch the previous index initialization and the index +=1 below. (just a side note: you don't need the semicolon :)) > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:354 > + if match and match.group('TEST') not in tests.keys(): > + tests[match.group('TEST')] = index > + elif match and 'SKIP' in match.group('MODIFIER'): > + _log.warn('The %s test from test_expectations.txt in line %d is also in Skipped!' % > + (match.group('TEST'),tests[match.group('TEST')] + 1)) > + tests[match.group('TEST')] = index something feels weird here but I don't know yet what.. > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:359 > + tests_list = tests.values().sort() > + for line in sorted(tests.values()): > + result += (expectations.split('\n')[line]) + '\n' > + return result Is it necessary to sourt the test.values() twice? Also you preform a split on the expectations on each iteration, you could move the split out of the for loop. A better solution would be: result_list = [expectations_list[line] for line in sorted(test.values())] return '\n'.join(result_list)

Created attachment 111611 [details] proposed fix

I suspect we don't want to make this change, but I've CCed the experts.

(In reply to comment #4) > I suspect we don't want to make this change, but I've CCed the experts. We do want, because Qt layout testing is regularly broken when somebody add a test to test_expectations.txt when a test is in the Skipped list already. Skipped list entry must be stronger than test_expectations.txt entry.

Comment on attachment 111611 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=111611&action=review > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:350 > + _log.warn('The %s test from test_expectations.txt in line %d is also in Skipped!' % If I'm following this patch correctly, the expectations that are passed in contain the concatenation of the contents of the Skipped files *and* the contents of the test_expectations.txt file, which means that (a) your line numbers are wrong, (b) you will fail if the test is mentioned multiple times in the Skipped files, (c) you will fail if the test is mentioned multiple times in the test_expectations.txt file. This patch should have tests for all of these cases - I'm r-'ing this patch for this reason at least. In addition, I *really* don't like embedding knowledge of the syntax of the expectations file into the port/* classes; it's a layering violation and may make it harder to evolve things in the future. The whole concept of handling Skipped files this way in the first place was supposed to be a temporary hack until we could transition everyone to test_expectations.txt files. Now, I realize that we probably don't have consensus over whether we even want to do that, or how, but we should probably fix that instead of continuing to work around it here. At the very least, I would be inclined to change the code so that the port object can return a list of tests to skip, and the generic code can decide how to merge the skip lists with any test_expectations. All that said, I don't think I even agree with the basic premise. I believe that it should be an error if the test is mentioned in both a Skipped file and the test_expectations.txt file, because such a situation indicates that you don't have a consistent view of what you expect the tests to do. So perhaps I'm not following what situation you are trying to handle? Sorry! I'm sure if I can get a better understanding of what you're trying to fix / workaround we can figure out a way to make it work that I'll be happy with.

(In reply to comment #6) > (From update of attachment 111611 [details]) View in context: > https://bugs.webkit.org/attachment.cgi?id=111611&action=review > > > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:350 > > + _log.warn('The %s test from test_expectations.txt in line %d is also in Skipped!' % > > If I'm following this patch correctly, the expectations that are passed in contain the concatenation of the contents of the Skipped files *and* th$ (a) the line number is good because the list from the Skipped files is appended at the end of the expectations string expectations = self._filesystem.read_text_file(expectations_path) + expectations (b) the list from the Skipped files is came as a set skipped_layout_tests() (c) the warn is written in multiple times in that case. I thought it is better than if I write it once. > > This patch should have tests for all of these cases - I'm r-'ing this patch for this reason at least. > > In addition, I *really* don't like embedding knowledge of the syntax of the expectations file into the port/* classes; it's a layering violation a$ I agree with your opinion, thank you for the r-, it needs a sophisticated solution. I think the merging should be in the test_expectations.py, what do you think? > > All that said, I don't think I even agree with the basic premise. I believe that it should be an error if the test is mentioned in both a Skipped $ > > So perhaps I'm not following what situation you are trying to handle? > we can't use test_expectations at now because some qt port still have to be run with orwt so we have a great list in the Skipped files, if a careless gardener use that it can broke the nrwt other example the test_expectations doesn't support the different qt versions so we need to use their Skipped if some test start to fail with it (qt-4.8, qt-5.0) > Sorry! I'm sure if I can get a better understanding of what you're trying to fix / workaround we can figure out a way to make it work that I'll be$

(In reply to comment #7) > (In reply to comment #6) > > (From update of attachment 111611 [details] [details]) View in context: > > https://bugs.webkit.org/attachment.cgi?id=111611&action=review > > > > > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:350 > > > + _log.warn('The %s test from test_expectations.txt in line %d is also in Skipped!' % > > > > If I'm following this patch correctly, the expectations that are passed in contain the concatenation of the contents of the Skipped files *and* th$ > (a) the line number is good because the list from the Skipped files is appended at the end of the expectations string > expectations = self._filesystem.read_text_file(expectations_path) + expectations Ah, right. > (b) the list from the Skipped files is came as a set > skipped_layout_tests() Okay. > (c) the warn is written in multiple times in that case. I thought it is better than if I write it once. That's probably okay. > > > > This patch should have tests for all of these cases - I'm r-'ing this patch for this reason at least. > > > > In addition, I *really* don't like embedding knowledge of the syntax of the expectations file into the port/* classes; it's a layering violation a$ > I agree with your opinion, thank you for the r-, it needs a sophisticated solution. > I think the merging should be in the test_expectations.py, what do you think? Yes, that would be the right place for it. > > > > All that said, I don't think I even agree with the basic premise. I believe that it should be an error if the test is mentioned in both a Skipped $ > > > > So perhaps I'm not following what situation you are trying to handle? > > > we can't use test_expectations at now because some qt port still have to be run with orwt so we have a great list in the Skipped files, if a careless gardener use that it can broke the nrwt The gardener should run new-run-webkit-tests --lint-test-files in order to ensure that it doesn't break; this is no different from handling any other typo or duplicate. If there is a conflict in the test_expectations.txt file, the gardener should be able to comment out or remove the line at that point. > other example the test_expectations doesn't support the different qt versions so we need to use their Skipped if some test start to fail with it (qt-4.8, qt-5.0) In this case, we should just add support for the qt versions to your port's implementation in NRWT. That should be very easy to do.

Created attachment 112675 [details] proposed fix

Comment on attachment 112675 [details] proposed fix I forgot to run the test_webkitpy, and I broke that.

Created attachment 113296 [details] proposed fix

(In reply to comment #11) > Created an attachment (id=113296) [details] > proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=113296&action=review > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:362 > + def skipped_test(self): > + return skipped_layout_tests Where does this 'skipped_layout_tests' comes from?

(In reply to comment #12) > (In reply to comment #11) > > Created an attachment (id=113296) [details] [details] > > proposed fix > > View in context: https://bugs.webkit.org/attachment.cgi?id=113296&action=review > > > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:362 > > + def skipped_test(self): > > + return skipped_layout_tests > > Where does this 'skipped_layout_tests' comes from? On WebKitPort based ports the skipped_layout_tests() return with a set of the skipped tests, but in the Chromium this method use a test_expectation object, but I want to use this set in the test_expectation's constructor to add skipped lines so I have to make a new method in the webkit.py what only return with the set of skipped tests to make the unittest happy, and don't kill the nrwt when it run chromium

(In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #11) > > > Created an attachment (id=113296) [details] [details] [details] > > > proposed fix > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=113296&action=review > > > > > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:362 > > > + def skipped_test(self): > > > + return skipped_layout_tests > > > > Where does this 'skipped_layout_tests' comes from? > > On WebKitPort based ports the skipped_layout_tests() return with a set of the skipped tests, but in the Chromium this method use a test_expectation object, but I want to use this set in the test_expectation's constructor to add skipped lines > so I have to make a new method in the webkit.py what only return with the set of skipped tests to make the unittest happy, and don't kill the nrwt when it run chromium But this is not a method call, also if that's the case the it should be 'self.skipped_layout_tests()', or am I missing something?

(In reply to comment #14) > But this is not a method call, also if that's the case the it should be 'self.skipped_layout_tests()', or am I missing something? Indeed I was wrong, but strange it didn't throw any exception

(In reply to comment #15) > (In reply to comment #14) > > But this is not a method call, also if that's the case the it should be 'self.skipped_layout_tests()', or am I missing something? > > Indeed I was wrong, but strange it didn't throw any exception Now I got it why didn't throw anything, I'am fixing it :)

Created attachment 113305 [details] proposed fix

Comment on attachment 113305 [details] proposed fix This is definitely getting closer ... View in context: https://bugs.webkit.org/attachment.cgi?id=113305&action=review > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:703 > + self._add_skipped_tests(port.skipped_tests()) Can't we just use port.skipped_layout_tests() here and not have to add a separate function? > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:836 > + _log.warn('The %s test from test_expectations.txt in line %d is also in Skipped!' % As per earlier discussion, I think this should probably be an error, not a warning. Also, can you add some unit tests to the test_expectations_unittest.py class?

(In reply to comment #18) > (From update of attachment 113305 [details]) > This is definitely getting closer ... > > View in context: https://bugs.webkit.org/attachment.cgi?id=113305&action=review > > > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:703 > > + self._add_skipped_tests(port.skipped_tests()) > > Can't we just use port.skipped_layout_tests() here and not have to add a separate function? In the chromium port the skipped_layout_tests() use a TestExpectation object so it fall in infinite recursive loop > > > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:836 > > + _log.warn('The %s test from test_expectations.txt in line %d is also in Skipped!' % > > As per earlier discussion, I think this should probably be an error, not a warning. > Ok, then it will be an error. > Also, can you add some unit tests to the test_expectations_unittest.py class? Sure

(In reply to comment #19) > > > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:703 > > > + self._add_skipped_tests(port.skipped_tests()) > > > > Can't we just use port.skipped_layout_tests() here and not have to add a separate function? > > In the chromium port the skipped_layout_tests() use a TestExpectation object so it fall in infinite recursive loop > Oh yeah, that call returns the list of skipped tests including the tests skipped in test_expectations.txt; that call shouldn't be on the port object at all. Oh well, that's a separate bug. Your solution is fine for now.

Created attachment 114218 [details] proposed fix with unit test I made a unit test

ping for review. :)

Comment on attachment 114218 [details] proposed fix with unit test View in context: https://bugs.webkit.org/attachment.cgi?id=114218&action=review Sorry, I'm not sure how this fell off of my plate. Almost there, I would just simplify the unit test a bit. > Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py:269 > + self.assertEqual(logs_string, 'The fast/foo/bar.html test from test_expectations.txt in line 2 is also in Skipped!\n') I don't usually test for exact string matches on the error messages; that makes the tests fragile. I would just test that parsing the expectations raises a ParseError, as in test_overrides__duplicate on line 226.

Created attachment 116659 [details] proposed fix Hi I simplified the unit test a little, but it doesn't throw any exception just log it as error.

Comment on attachment 116659 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=116659&action=review > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:837 > + (test.name, index)) This needs to add an error to a list of errors, not call _log.error(); that way it can be included in the total list of errors during the call to _report_errors. That's why a ParseError() isn't getting raised. Unfortunately, the refactoring of this class has made adding an error harder than it used to be, as there isn't a global list-of-errors object. maybe create a self.skipped_tests_errors, and check that in self._report_errors()? Also, if these are truly errors (and not warnings), you should probably not be adding them to the model. > Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py:263 > + self._exp = TestExpectations(port, 'failures/expected/text.html\n', 'BUGX : failures/expected/text.html = text\n', None) This should be raising a ParseError; see comments above.

(In reply to comment #25) > (From update of attachment 116659 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=116659&action=review > > > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:837 > > + (test.name, index)) > > This needs to add an error to a list of errors, not call _log.error(); that way it can be included in the total list of errors during the call to _report_errors. That's why a ParseError() isn't getting raised. > > Unfortunately, the refactoring of this class has made adding an error harder than it used to be, as there isn't a global list-of-errors object. maybe create a self.skipped_tests_errors, and check that in self._report_errors()? > Ok, I see > Also, if these are truly errors (and not warnings), you should probably not be adding them to the model. > > > Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py:263 > > + self._exp = TestExpectations(port, 'failures/expected/text.html\n', 'BUGX : failures/expected/text.html = text\n', None) > > This should be raising a ParseError; see comments above.

One more accident why we really need this change as soon as possible: http://trac.webkit.org/changeset/101745 Kristóf, could you speed up fixing this bug, please? Maybe it would be more effective if you guys continue reviewing/fixing this patch with IRC talking instead of waiting days for each other. Thanks your work in advance.

I can see if I can post a patch in the next day or two that addresses my last concerns, if Kristóf doesn't get to it first.

Created attachment 117663 [details] proposed fix Now it raise a ParseError if it is run in lint mode.

Comment on attachment 117663 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=117663&action=review > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:791 > + Minor nit ... _is_lint_mode makes warnings errors. If you want these to fail only in lint mode, I'd just change these to _skipped_tests_warnings and append them to the warnings list. CQ-'ing just for that. There is actually a separate discussion about making the expectations errors less fatal going on in https://bugs.webkit.org/show_bug.cgi?id=73603 ; fixing that bug might've made this bug unnecessary, but I think there's good work in this patch to clean up our handling of skipped files regardless.

I changed the name of the list patch landed in: http://trac.webkit.org/changeset/101979