Bug 71067 – Test expectation errors should include the file name and platform in both the log and exception info.

• Component: New Bugs
• Status: RESOLVED FIXED

[David Levin]

Test expectation errors should include the file name and platform in both the log and exception info.

[David Levin]

Created attachment 112764 [details]
Patch

[David Levin]

Added smfr as fyi. A follow up on https://bugs.webkit.org/show_bug.cgi?id=71033#c10

Everyone else is a possible reviewer :)

[Dirk Pranke]

View in context: https://bugs.webkit.org/attachment.cgi?id=112764&action=review

I'm curious as to what the motivation for this patch is? Since it isn't distinguishing between regular expectations and overrides, I'm guessing you're doing something where you want to see which port's expectations file you're using? In the normal run-webkit-tests case, is this necessary? Or are you doing something like linting the test files where you're likely to encounter multiple expectations files?

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:790
> +            failure_title = "FAILURES FOR %s in %s" % (str(self._test_config), test_expectation_relative_path)

You can use test_expectation_relative_path = self._port.relative_test_filename(self._port.path_to_test_expectations_file()) as long as the expectations file is under LayoutTests (which AFAIK they all are). That won't include the "LayoutTests" part, which may or may not be desirable.

[David Levin]

(In reply to comment #3)
> View in context: https://bugs.webkit.org/attachment.cgi?id=112764&action=review
> 
> I'm curious as to what the motivation for this patch is? Since it isn't distinguishing between regular expectations and overrides, I'm guessing you're doing something where you want to see which port's expectations file you're using? In the normal run-webkit-tests case, is this necessary? Or are you doing something like linting the test files where you're likely to encounter multiple expectations files?

Linting the test files.

For example, this happens during a unit test run right now on OS X. Now this is a normal OS X (not Chromium OS X) bot, so when we got a failure, I tried to look at the test_expectations for platform/mac but then found that it was complaining about the chromium one.

Also in general if a test case is failing due to this, it would be nice to know which test_expectations is causing it (since we have several of them checked in).



> 
> > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:790
> > +            failure_title = "FAILURES FOR %s in %s" % (str(self._test_config), test_expectation_relative_path)
> 
> You can use test_expectation_relative_path = self._port.relative_test_filename(self._port.path_to_test_expectations_file()) as long as the expectations file is under LayoutTests (which AFAIK they all are). That won't include the "LayoutTests" part, which may or may not be desirable.

I like the path from the root as it feels less "magical". I see LayoutTests/ and I know where that is. If I see "platform/", I'm not immediately sure since there are multiple platform dirs.

[Dirk Pranke]

(In reply to comment #4)
> (In reply to comment #3)
> > View in context: https://bugs.webkit.org/attachment.cgi?id=112764&action=review
> > 
> > I'm curious as to what the motivation for this patch is? Since it isn't distinguishing between regular expectations and overrides, I'm guessing you're doing something where you want to see which port's expectations file you're using? In the normal run-webkit-tests case, is this necessary? Or are you doing something like linting the test files where you're likely to encounter multiple expectations files?
> 
> Linting the test files.
> 
> For example, this happens during a unit test run right now on OS X. Now this is a normal OS X (not Chromium OS X) bot, so when we got a failure, I tried to look at the test_expectations for platform/mac but then found that it was complaining about the chromium one.
> 
> Also in general if a test case is failing due to this, it would be nice to know which test_expectations is causing it (since we have several of them checked in).
> 

Perhaps. I'm on the fence as to whether this will be unnecessary noise or not in the common case, but adding one line is not a big deal and it's clearly useful in the linting-the-files case.

> 
> > 
> > > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:790
> > > +            failure_title = "FAILURES FOR %s in %s" % (str(self._test_config), test_expectation_relative_path)
> > 
> > You can use test_expectation_relative_path = self._port.relative_test_filename(self._port.path_to_test_expectations_file()) as long as the expectations file is under LayoutTests (which AFAIK they all are). That won't include the "LayoutTests" part, which may or may not be desirable.
> 
> I like the path from the root as it feels less "magical". I see LayoutTests/ and I know where that is. If I see "platform/", I'm not immediately sure since there are multiple platform dirs.

Sure, I figured you'd say something like that :) That's fine, just thought I'd mention the alternative.

[David Levin]

(In reply to comment #5)
> Perhaps. I'm on the fence as to whether this will be unnecessary noise or not in the common case, but adding one line is not a big deal and it's clearly useful in the linting-the-files case.

Yeah, checkout https://bugs.webkit.org/show_bug.cgi?id=71033#c0

I really dislike tests that fail and leave you guessing about how to fix them so this should help.

[WebKit Review Bot]

Comment on attachment 112764 [details]
Patch

Clearing flags on attachment: 112764

Committed r98666: <http://trac.webkit.org/changeset/98666>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.