Bug 36771 – new-run-webkit-tests shouldn't report "unexpected passes" when pixel tests are disabled

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Dirk Pranke]

Currently, if you have a test marked as expected to fail with an IMAGE failure, and you disable pixel tests, you'll get "unexpected passes". Strictly speaking, this isn't true, because you would expect this test to pass. More importantly, it's confusing if you're trying to get new-run-webkit-tests's output to match run-webkit-tests's default output (which has pixel tests disabled).

[Dirk Pranke]

Created attachment 51956 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 51956 [details]
Patch

ok.

[Ojan Vafai]

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

[Dirk Pranke]

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

[Dirk Pranke]

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

[Dirk Pranke]

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

[Eric Seidel (no email)]

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?

[Dirk Pranke]

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

[Dirk Pranke]

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

[Dirk Pranke]

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

[Eric Seidel (no email)]

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.

[Dirk Pranke]

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

[Dirk Pranke]

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.

[Ojan Vafai]

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

LGTM

[Eric Seidel (no email)]

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.

[Dirk Pranke]

(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 :)

[Dirk Pranke]

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