Bug 38796 - DryrunTest fails on every platform other than mac
Summary: DryrunTest fails on every platform other than mac
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-07 22:56 PDT by Eric Seidel (no email)
Modified: 2010-05-11 17:58 PDT (History)
6 users (show)

See Also:


Attachments
Patch (1.99 KB, patch)
2010-05-07 22:59 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch (3.85 KB, patch)
2010-05-10 16:54 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
Patch (3.48 KB, patch)
2010-05-10 18:39 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
Patch (3.56 KB, patch)
2010-05-10 18:57 PDT, Dirk Pranke
eric: review+
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) 2010-05-07 22:56:12 PDT
DryrunTest fails on every platform other than mac
Comment 1 Eric Seidel (no email) 2010-05-07 22:57:48 PDT
Windows can be fixed.  But linux can't.  We should re-write the test, or change how dryrun works to not assume Chromium in the linux case.

For now I'm going to disable the test on non-mac to make the bots green.
Comment 2 Eric Seidel (no email) 2010-05-07 22:59:13 PDT
Created attachment 55461 [details]
Patch
Comment 3 Nikolas Zimmermann 2010-05-07 23:00:18 PDT
Comment on attachment 55461 [details]
Patch

LGTM, r=me.
Comment 4 Eric Seidel (no email) 2010-05-07 23:04:44 PDT
Committed r58999: <http://trac.webkit.org/changeset/58999>
Comment 5 Eric Seidel (no email) 2010-05-07 23:05:46 PDT
Comment on attachment 55461 [details]
Patch

webkit-patch land --no-close
should have obsoleted this patch.  Something must have regressed in webkit-patch.
Comment 6 Dirk Pranke 2010-05-10 16:54:32 PDT
Created attachment 55623 [details]
Patch
Comment 7 Dirk Pranke 2010-05-10 16:55:35 PDT
I re-wrote the test and fixed what was breaking on chromium windows. If we want to test more ports we can add them now.
Comment 8 Eric Seidel (no email) 2010-05-10 18:29:59 PDT
Comment on attachment 55623 [details]
Patch

LGTM.  Why does the mac port need to be run dryrun on mac only?
Comment 9 Dirk Pranke 2010-05-10 18:31:16 PDT
Grr. Actually, this patch is bad, because it'll still try to run the chromium tests in a non-chromium checkout and fail.
Comment 10 Dirk Pranke 2010-05-10 18:39:42 PDT
Created attachment 55637 [details]
Patch
Comment 11 Dirk Pranke 2010-05-10 18:41:18 PDT
(In reply to comment #8)
> (From update of attachment 55623 [details])
> LGTM.  Why does the mac port need to be run dryrun on mac only?

If you run it on windows, the mac port tries to pull in server_process which pulls in fcntl, which doesn't exist.
Comment 12 Dirk Pranke 2010-05-10 18:42:25 PDT
It seems like there should be a better way to get coverage of the platforms other than the one you're running on, but I'm not sure what it is.

Also, for platforms with multiple implementations (e.g., mac, chromium-mac) we need a way to figure out which is which.
Comment 13 Eric Seidel (no email) 2010-05-10 18:44:55 PDT
Comment on attachment 55637 [details]
Patch

We should add a comment in the code as to why mac is run on mac-only.  Your explanation seemed to suggest it should be run on everything except for "win32".

Also:

+        if sys.platform not in 'mac':

Why change the quotes?  For better or worse our python guidelines in the wiki suggest " over ': http://trac.webkit.org/wiki/PythonGuidelines
Comment 14 Dirk Pranke 2010-05-10 18:57:10 PDT
Created attachment 55640 [details]
Patch
Comment 15 Dirk Pranke 2010-05-10 18:57:33 PDT
(In reply to comment #13)
> (From update of attachment 55637 [details])
> We should add a comment in the code as to why mac is run on mac-only.  Your explanation seemed to suggest it should be run on everything except for "win32".
> 
> Also:
> 
> +        if sys.platform not in 'mac':
> 
> Why change the quotes?  For better or worse our python guidelines in the wiki suggest " over ': http://trac.webkit.org/wiki/PythonGuidelines

That was not intentional. Fixed.
Comment 16 Eric Seidel (no email) 2010-05-10 19:02:04 PDT
Comment on attachment 55640 [details]
Patch

LGTM.

Seems we still could make the check be != "win32" no?
Comment 17 Eric Seidel (no email) 2010-05-10 19:02:46 PDT
Comment on attachment 55640 [details]
Patch

Oh, and I should have fixed:
sys.platform not in "mac"

that shoudl read "sys.platform != "mac""

not in happens to work but is not as intended.
Comment 18 Dirk Pranke 2010-05-10 19:03:45 PDT
(In reply to comment #17)
> (From update of attachment 55640 [details])
> Oh, and I should have fixed:
> sys.platform not in "mac"
> 
> that shoudl read "sys.platform != "mac""
> 
> not in happens to work but is not as intended.

I was wondering about that. 

As to the != 'win32', I dunno; I haven't tested this on non-chromium linux. I think it'll work.
Comment 19 Dirk Pranke 2010-05-11 16:22:27 PDT
Committed r59182: <http://trac.webkit.org/changeset/59182>
Comment 20 WebKit Review Bot 2010-05-11 17:58:16 PDT
http://trac.webkit.org/changeset/59182 might have broken GTK Linux 32-bit Debug
The following changes are on the blame list:
http://trac.webkit.org/changeset/59182
http://trac.webkit.org/changeset/59183