Summary: | new-run-webkit-tests shouldn't report "unexpected passes" when pixel tests are disabled | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dirk Pranke <dpranke> | ||||||||||||
Component: | Tools / Tests | Assignee: | Dirk Pranke <dpranke> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | dglazkov, ojan, senorblanco | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | PC | ||||||||||||||
OS: | OS X 10.5 | ||||||||||||||
Attachments: |
|
Description
Dirk Pranke
2010-03-29 12:24:11 PDT
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> |