Bug 97225

Summary: make Skip, WontFix be the only expectations on a line
Product: WebKit Reporter: Dirk Pranke <dpranke>
Component: Tools / TestsAssignee: Dirk Pranke <dpranke>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, jberlin, ojan, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
add MISSING_BUG_WARNING constant for string reuse
none
patch for landing - move warning ojan: review+

Description Dirk Pranke 2012-09-20 09:25:49 PDT
Per the design docs in http://trac.webkit.org/wiki/TestExpectations, [ Skip ] and [ WontFix ] are not supposed to allow any other modifiers, but I forgot to implement that before we converted the syntax.
Comment 1 Dirk Pranke 2012-09-20 14:51:12 PDT
Created attachment 164989 [details]
Patch
Comment 2 Dirk Pranke 2012-09-20 14:59:32 PDT
Committed r129171: <http://trac.webkit.org/changeset/129171>
Comment 3 Ojan Vafai 2012-09-20 15:22:25 PDT
Comment on attachment 164989 [details]
Patch

Don't we need to skip WontFix tests first? Otherwise, you'll get unexpected failures for WontFix tests.
Comment 4 Dirk Pranke 2012-09-20 15:31:20 PDT
WontFix implies Skip automatically (in the code today).
Comment 5 Dirk Pranke 2012-09-20 15:32:02 PDT
in the new syntax; in the old syntax, it didn't, i.e., when we translate the new syntax back into the old syntax, we add on a "SKIP".
Comment 6 Dirk Pranke 2012-09-20 15:33:26 PDT
making WONTFIX imply SKIP regardless of the syntax is what I'm working on right now; it unfortunately requires updating a bunch of unit tests in non trivial ways.
Comment 7 Dirk Pranke 2012-09-20 15:35:30 PDT
At least, I think the existing code does this; if not, I just broke the world ;).
Comment 8 Dirk Pranke 2012-09-20 16:56:22 PDT
Reopening to attach new patch.
Comment 9 Dirk Pranke 2012-09-20 16:56:24 PDT
Created attachment 165014 [details]
Patch
Comment 10 Dirk Pranke 2012-09-20 16:57:08 PDT
Note that the changed TestExpectations files were landed in http://trac.webkit.org/changeset/129171
Comment 11 Ojan Vafai 2012-09-20 17:04:06 PDT
Comment on attachment 165014 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:442
> +        return self.warnings and self.warnings != ['Test lacks BUG modifier.']

This is a bit too hacky I think. Could we at least move the string to a shared constant that both locations use?
Comment 12 Dirk Pranke 2012-09-20 17:48:13 PDT
(In reply to comment #11)
> (From update of attachment 165014 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165014&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:442
> > +        return self.warnings and self.warnings != ['Test lacks BUG modifier.']
> 
> This is a bit too hacky I think. Could we at least move the string to a shared constant that both locations use?

Sure, good idea.
Comment 13 Dirk Pranke 2012-09-20 17:53:19 PDT
Created attachment 165023 [details]
add MISSING_BUG_WARNING constant for string reuse
Comment 14 Dirk Pranke 2012-09-20 18:26:00 PDT
Created attachment 165029 [details]
patch for landing - move warning
Comment 15 Dirk Pranke 2012-09-20 18:33:10 PDT
Committed r129184: <http://trac.webkit.org/changeset/129184>
Comment 16 Simon Fraser (smfr) 2012-09-20 20:52:29 PDT
This seems to have caused a webkitpy failure:
http://build.webkit.org/builders/Apple%20MountainLion%20%28Leaks%29/builds/131/steps/webkitpy-test/logs/stdio
Comment 17 Simon Fraser (smfr) 2012-09-20 21:00:04 PDT
Commented out the failing unit test in http://trac.webkit.org/changeset/129188
Comment 18 Dirk Pranke 2012-09-21 12:23:05 PDT
(In reply to comment #17)
> Commented out the failing unit test in http://trac.webkit.org/changeset/129188

Ah, I forgot to change that to assertTrue(). Thanks!
Comment 19 Dirk Pranke 2012-09-21 12:34:57 PDT
Test fixed in http://trac.webkit.org/changeset/129245 .