Bug 36771

Created attachment 51956 [details] Patch

Comment on attachment 51956 [details] Patch ok.

Comment on attachment 51956 [details] Patch > + def _matches_an_expected_result(self, test, result): > + """Returns whether we got one of the expected results for this test.""" > + expected_failures = self._expectations.get_expectations(test) Isn't this modifying the list inside self._expectations? I think you should make a copy of this list ([:]) and modify that. > + if self._options.no_pixel_tests: > + if test_expectations.IMAGE in expected_failures: > + expected_failures.remove(test_expectations.IMAGE) > + expected_failures.add(test_expectations.PASS) > + if test_expectations.IMAGE_PLUS_TEXT in expected_failures: > + expected_failures.remove(test_expectations.IMAGE_PLUS_TEXT) > + expected_failures.add(test_expectations.TEXT) > + return ((result in expected_failures) or > + (result in (test_expectations.IMAGE, test_expectations.TEXT, > + test_expectations.IMAGE_PLUS_TEXT) and > + test_expectations.FAIL in expected_failures) or > + (result == test_expectations.MISSING and > + self.is_rebaselining(test)) or > + (result == test_expectations.SKIP and > + self._expected_failures.has_modifier(test, > + test_expectations.SKIP))) > + > def _display_one_line_progress(self, result_summary): > """Displays the progress through the test run.""" > self._meter.update("Testing: %d ran as expected, %d didn't, %d left" %

(In reply to comment #3) > (From update of attachment 51956 [details]) > > + def _matches_an_expected_result(self, test, result): > > + """Returns whether we got one of the expected results for this test.""" > > + expected_failures = self._expectations.get_expectations(test) > > Isn't this modifying the list inside self._expectations? I think you should > make a copy of this list ([:]) and modify that. > This is actually a set, but otherwise I think you're right. I'll make a copy.

Created attachment 51967 [details] only modify a copy of the expectation - thanks Ojan!

*** Bug 36689 has been marked as a duplicate of this bug. ***

Comment on attachment 51967 [details] only modify a copy of the expectation - thanks Ojan! I would have probably written those as a series of ifs, instead of one long if with a bunch of ors. Your ChangeLog is not at the top of the file, which will cause badness and confusion. :) resolve-ChangeLogs (optionally --force) is your friend. Git is not friendly to changelogs. :( Wow, this is also really noisy now that it's been moved out of test_expectations. Was much easier to read when every other word wasn't test_expectations. :) Does TestRunner have a self._expected_failures like TestExpecations does?

> (From update of attachment 51967 [details]) > I would have probably written those as a series of ifs, instead of one long if > with a bunch of ors. > > Your ChangeLog is not at the top of the file, which will cause badness and > confusion. :) resolve-ChangeLogs (optionally --force) is your friend. Git is > not friendly to changelogs. :( > Normally I have resolve-ChangeLogs run as part of the merge handler. I'm not sure what happened here. > Wow, this is also really noisy now that it's been moved out of > test_expectations. Was much easier to read when every other word wasn't > test_expectations. :) > Agreed. That's why it was in test_expectations in the first place. I wish there was an easy way to import just the constants. > Does TestRunner have a self._expected_failures like TestExpecations does? > No. Good catch!

Created attachment 51978 [details] fix changelog entry, fix typo from review by eseidel.

Created attachment 51983 [details] update w/ feedback from eseidel

Comment on attachment 51983 [details] update w/ feedback from eseidel I'm confused as to how this line: 725 if result in expected_failures: and this line: 734 if (result == test_expectations.SKIP and actually are dealing with the same "result" variable. One seems like it should be a path, the other seems like it should be some sort of ENUM. I might have put this sort of translation code (including the copy) inside test_expectations.py to limit the number of times that you need to write "test_expecations": 718 expected_failures = expected_failures.copy() 719 if test_expectations.IMAGE in expected_failures: 720 expected_failures.remove(test_expectations.IMAGE) 721 expected_failures.add(test_expectations.PASS) 722 if test_expectations.IMAGE_PLUS_TEXT in expected_failures: 723 expected_failures.remove(test_expectations.IMAGE_PLUS_TEXT) 724 expected_failures.add(test_expectations.TEXT) That and it's relatively self-contained from the rest of the function. Actually.. if you ahve some sort of epectations_ignoring_pixel_failures() translation function, can't we then just pipe those results into some very slightly modified version of the original matches_an_expected_result which takes an expecations? I mean, in general the change looks OK. But the "result" confusion makes me wonder if part of it is wrong.

(In reply to comment #11) > (From update of attachment 51983 [details]) > I'm confused as to how this line: > 725 if result in expected_failures: > > and this line: > 734 if (result == test_expectations.SKIP and > > actually are dealing with the same "result" variable. > > One seems like it should be a path, the other seems like it should be some sort > of ENUM. I see. It looks like a bad choice of variable name has confused you. Okay, I'm going to restructure this patch again and resubmit.

Created attachment 51996 [details] update after more feedback from eseidel Okay, I'm adopting a different approach here. We'll keep as much of the logic in test_expectations as possible, but make the decision of how to handle pixel test failures passed in from run_webkit_tests. Also, I've split the logic out into separate testable pure function calls, and added more unit tests.

Comment on attachment 51996 [details] update after more feedback from eseidel LGTM

Comment on attachment 51996 [details] update after more feedback from eseidel You compute "test_is_skipped" here but never use it? 165 test_is_skipped = self._expected_failures.has_modifier(test, SKIP) I don't really understand the use of the name "t", "m" and "f". I don't think they add to readability being so short. In general this looks great and much cleaner than before. Thank you for workign through it again. r=me. Please fix some subset of the above nits when you go to land it. No need to post it again.

(In reply to comment #15) > (From update of attachment 51996 [details]) > You compute "test_is_skipped" here but never use it? > 165 test_is_skipped = self._expected_failures.has_modifier(test, SKIP) > This line was unnecessary and left over from a prior rev. I've deleted it. > I don't really understand the use of the name "t", "m" and "f". I don't think > they add to readability being so short. I've renamed m() to match() and changed t() and f() to self.assertTrue(match()) and self.assertFalse(match()). That's as far as I'm willing to go, longer-names-wise :)

Committed r56871: <http://trac.webkit.org/changeset/56871>