Bug 63838

Summary: new-run-webkit-tests results does not understand that mac uses test_expectations files
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: Tools / TestsAssignee: Eric Seidel (no email) <eric>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dpranke, ojan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 34984    
Attachments:
Description Flags
Patch none

Eric Seidel (no email)
Reported 2011-07-01 13:17:09 PDT
new-run-webkit-tests does not understand that mac uses test_expectations files When uses_expectations_file is false, these failures appear for mac: test results +fast/canvas/webgl/gl-teximage.html expected actual diff pretty diff +http/tests/cookies/third-party-cookie-relaxing.html expected actual diff pretty diff +inspector/timeline/timeline-layout.html expected actual diff pretty diff +plugins/npp-set-window-called-during-destruction.html expected actual diff pretty diff Yet they're all mentioned in platform/mac/test_expectations.txt: BUGWK58192 : plugins/npp-set-window-called-during-destruction.html = TEXT // Flaky tests when run multi-process BUGWK58192 : fast/dom/frame-loading-via-document-write.html = TEXT PASS BUGWK58192 : http/tests/appcache/fail-on-update-2.html = TEXT PASS BUGWK58192 : http/tests/appcache/fail-on-update.html = TEXT PASS BUGWK58192 : http/tests/inspector/console-websocket-error.html = TEXT PASS BUGWK58192 : fast/canvas/webgl/gl-teximage.html = TEXT PASS BUGWK58192 : fast/frames/flattening/iframe-flattening-offscreen.html = TEXT PASS BUGWK58192 : svg/dom/SVGScriptElement/script-set-href.svg = TEXT PASS Part of the problem is in manager.py: # FIXME: If non-chromium ports start using an expectations file, # we should make this check more robust. results['uses_expectations_file'] = port_obj.name().find('chromium') != -1 however, I dont' think we want to set that bool as it does more than we want for mac. Maybe we can't have our cake and eat it too. WE should probably just move those to the Skipped list for Mac. But platform/mac/test_expetations.txt was a useful tool for the ORWT->NRWT conversion as it's just tests which fail in NRWT but don't in ORWT.
Attachments
Patch (5.66 KB, patch)
2011-07-01 14:07 PDT, Eric Seidel (no email) 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Dirk Pranke
Comment 1 2011-07-01 13:43:10 PDT
You can probably just replace that line with: port_obj._filesystem.exists(port_obj.path_to_expectations_file())
Eric Seidel (no email)
Comment 2 2011-07-01 13:47:02 PDT
True, we could. I sympathize with what results.html is doing. It's being told not to expect test_expectations, and then I'm expecting it to filter out test_expetations-base failures. I suspect we should just move these all to the Skipped lists.
Eric Seidel (no email)
Comment 3 2011-07-01 14:07:49 PDT
Created attachment 99520 [details] Patch
Eric Seidel (no email)
Comment 4 2011-07-01 14:08:52 PDT
Thank you for the suggestion Dirk!
Dirk Pranke
Comment 5 2011-07-01 14:12:06 PDT
Comment on attachment 99520 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99520&action=review > Tools/Scripts/webkitpy/layout_tests/port/base.py:718 > + I probably wouldn't have introduced a separate function just for this, since it's currently only called in one place ... > Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py:264 > + def test_uses_test_expectations_file(self): In addition to doing the tests on the base class, I usually try to add a test in the port_testcase.py class that is then run on every subclass, to ensure the subclass is implementing/overridding the routine as expected.
Ojan Vafai
Comment 6 2011-07-01 14:14:43 PDT
It seems to me that we either need to use an expectations file with all the complexity it involves or just used Skipped lists. I don't see a reasonable middle ground off the top of my head. Doesn't this patch just make it so that Mac also uses an expectations file? I think that's fine, but I have heard strong resistance to this complexity from maintainers of the Apple port. I think this code change is correct regardless, but it still leaves open the question of whether Apple's port should have an expectations file.
Adam Barth
Comment 7 2011-07-01 14:18:12 PDT
> It seems to me that we either need to use an expectations file with all the complexity it involves or just used Skipped lists. I don't see a reasonable middle ground off the top of my head. We have have a less complex version of the test_expectations file. > Doesn't this patch just make it so that Mac also uses an expectations file? I think that's fine, but I have heard strong resistance to this complexity from maintainers of the Apple port. Nope. It uses both. > I think this code change is correct regardless, but it still leaves open the question of whether Apple's port should have an expectations file. All the ports should work the same way. We'll find something that works for everyone.
Ojan Vafai
Comment 8 2011-07-01 14:19:23 PDT
(In reply to comment #7) > > It seems to me that we either need to use an expectations file with all the complexity it involves or just used Skipped lists. I don't see a reasonable middle ground off the top of my head. > > We have have a less complex version of the test_expectations file. In what way? Just because it's shorter? The downside is that you then need to surface all the expectations information into the results.html page.
Adam Barth
Comment 9 2011-07-01 14:30:29 PDT
> In what way? Just because it's shorter? The downside is that you then need to surface all the expectations information into the results.html page. Sorry, I mean to say "we can have have a less complex version". The test_expectations.txt file is more complex than it needs to be for the job it does.
Dirk Pranke
Comment 10 2011-07-01 14:38:44 PDT
(In reply to comment #9) > > In what way? Just because it's shorter? The downside is that you then need to surface all the expectations information into the results.html page. > > Sorry, I mean to say "we can have have a less complex version". The test_expectations.txt file is more complex than it needs to be for the job it does. Are you saying that the test_expectations.txt file is attempting to do more than it should be doing (has too much functionality), or that the existing functionality could be implemented more simply? Examples for either?
Adam Barth
Comment 11 2011-07-01 14:40:59 PDT
This the wrong forum for this discussion.
WebKit Review Bot
Comment 12 2011-07-01 15:11:59 PDT
Comment on attachment 99520 [details] Patch Clearing flags on attachment: 99520 Committed r90285: <http://trac.webkit.org/changeset/90285>
WebKit Review Bot
Comment 13 2011-07-01 15:12:04 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.