Bug 71067 - Test expectation errors should include the file name and platform in both the log and exception info.
Summary: Test expectation errors should include the file name and platform in both the...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Levin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-27 15:00 PDT by David Levin
Modified: 2011-10-27 17:29 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.66 KB, patch)
2011-10-27 15:02 PDT, David Levin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Levin 2011-10-27 15:00:55 PDT
Test expectation errors should include the file name and platform in both the log and exception info.
Comment 1 David Levin 2011-10-27 15:02:54 PDT
Created attachment 112764 [details]
Patch
Comment 2 David Levin 2011-10-27 15:06:55 PDT
Added smfr as fyi. A follow up on https://bugs.webkit.org/show_bug.cgi?id=71033#c10

Everyone else is a possible reviewer :)
Comment 3 Dirk Pranke 2011-10-27 15:16:13 PDT
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.
Comment 4 David Levin 2011-10-27 15:28:32 PDT
(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.
Comment 5 Dirk Pranke 2011-10-27 15:35:27 PDT
(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.
Comment 6 David Levin 2011-10-27 15:38:08 PDT
(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.
Comment 7 WebKit Review Bot 2011-10-27 17:29:54 PDT
Comment on attachment 112764 [details]
Patch

Clearing flags on attachment: 112764

Committed r98666: <http://trac.webkit.org/changeset/98666>
Comment 8 WebKit Review Bot 2011-10-27 17:29:59 PDT
All reviewed patches have been landed.  Closing bug.