RESOLVED FIXED 97225
make Skip, WontFix be the only expectations on a line 
https://bugs.webkit.org/show_bug.cgi?id=97225
Summary make Skip, WontFix be the only expectations on a line
Dirk Pranke
Reported 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.
Attachments
Patch (241.47 KB, patch)
2012-09-20 14:51 PDT, Dirk Pranke 		no flags
DetailsFormatted DiffDiff
Patch (21.40 KB, patch)
2012-09-20 16:56 PDT, Dirk Pranke 		no flags
DetailsFormatted DiffDiff
add MISSING_BUG_WARNING constant for string reuse (22.36 KB, patch)
2012-09-20 17:53 PDT, Dirk Pranke 		no flags
DetailsFormatted DiffDiff
patch for landing - move warning (21.78 KB, patch)
2012-09-20 18:26 PDT, Dirk Pranke 		ojan: review+
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Dirk Pranke
Comment 1 2012-09-20 14:51:12 PDT
Created attachment 164989 [details] Patch
Dirk Pranke
Comment 2 2012-09-20 14:59:32 PDT
Committed r129171: <http://trac.webkit.org/changeset/129171>
Ojan Vafai
Comment 3 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.
Dirk Pranke
Comment 4 2012-09-20 15:31:20 PDT
WontFix implies Skip automatically (in the code today).
Dirk Pranke
Comment 5 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".
Dirk Pranke
Comment 6 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.
Dirk Pranke
Comment 7 2012-09-20 15:35:30 PDT
At least, I think the existing code does this; if not, I just broke the world ;).
Dirk Pranke
Comment 8 2012-09-20 16:56:22 PDT
Reopening to attach new patch.
Dirk Pranke
Comment 9 2012-09-20 16:56:24 PDT
Created attachment 165014 [details] Patch
Dirk Pranke
Comment 10 2012-09-20 16:57:08 PDT
Note that the changed TestExpectations files were landed in http://trac.webkit.org/changeset/129171
Ojan Vafai
Comment 11 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?
Dirk Pranke
Comment 12 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.
Dirk Pranke
Comment 13 2012-09-20 17:53:19 PDT
Created attachment 165023 [details] add MISSING_BUG_WARNING constant for string reuse
Dirk Pranke
Comment 14 2012-09-20 18:26:00 PDT
Created attachment 165029 [details] patch for landing - move warning
Dirk Pranke
Comment 15 2012-09-20 18:33:10 PDT
Committed r129184: <http://trac.webkit.org/changeset/129184>
Simon Fraser (smfr)
Comment 16 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
Simon Fraser (smfr)
Comment 17 2012-09-20 21:00:04 PDT
Commented out the failing unit test in http://trac.webkit.org/changeset/129188
Dirk Pranke
Comment 18 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!
Dirk Pranke
Comment 19 2012-09-21 12:34:57 PDT
Test fixed in http://trac.webkit.org/changeset/129245 .
Note You need to log in before you can comment on or make changes to this bug.