Bug 63838 - new-run-webkit-tests results does not understand that mac uses test_expectations files
Summary: new-run-webkit-tests results does not understand that mac uses test_expectati...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
Depends on:
Blocks: 34984
  Show dependency treegraph
 
Reported: 2011-07-01 13:17 PDT by Eric Seidel (no email)
Modified: 2011-07-01 15:12 PDT (History)
4 users (show)

See Also:


Attachments
Patch (5.66 KB, patch)
2011-07-01 14:07 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 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.
Comment 1 Dirk Pranke 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())
Comment 2 Eric Seidel (no email) 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.
Comment 3 Eric Seidel (no email) 2011-07-01 14:07:49 PDT
Created attachment 99520 [details]
Patch
Comment 4 Eric Seidel (no email) 2011-07-01 14:08:52 PDT
Thank you for the suggestion Dirk!
Comment 5 Dirk Pranke 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.
Comment 6 Ojan Vafai 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.
Comment 7 Adam Barth 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.
Comment 8 Ojan Vafai 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.
Comment 9 Adam Barth 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.
Comment 10 Dirk Pranke 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?
Comment 11 Adam Barth 2011-07-01 14:40:59 PDT
This the wrong forum for this discussion.
Comment 12 WebKit Review Bot 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>
Comment 13 WebKit Review Bot 2011-07-01 15:12:04 PDT
All reviewed patches have been landed.  Closing bug.