Bug 39058 - old-run-webkit-tests should use ensure-valid-python to check if it can run the websockets tests
Summary: old-run-webkit-tests should use ensure-valid-python to check if it can run th...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 38886 41612
Blocks:
  Show dependency treegraph
 
Reported: 2010-05-13 06:13 PDT by Eric Seidel (no email)
Modified: 2010-07-05 07:16 PDT (History)
2 users (show)

See Also:


Attachments
Patch (4.41 KB, patch)
2010-05-17 00:56 PDT, Fumitoshi Ukai
no flags Details | Formatted Diff | Diff
Patch (5.52 KB, patch)
2010-06-02 23:59 PDT, Fumitoshi Ukai
no flags Details | Formatted Diff | Diff
Patch (1.27 KB, patch)
2010-06-08 02:41 PDT, Fumitoshi Ukai
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-13 06:13:39 PDT
old-run-webkit-tests should use ensure-valid-python to check if it can run the websockets tests

Right now the tests are manually skipped for Tiger, which is bad.  Would be better to run them automatically when we can find python 2.5+ and not when we can't.
Comment 1 Eric Seidel (no email) 2010-05-13 06:14:35 PDT
ensure-valid-python will need to have a --non-interactive option or some flag to make it not try to install if missing.  Actually it should probably default to not installing if missing since it can't do so w/o a user sitting at the keyboard.
Comment 2 Alexey Proskuryakov 2010-05-13 09:08:42 PDT
I think that this should be WONTFIX. Skipping tests is bad - if we require developers to make the extra effort of installing custom python, this should be mandatory.
Comment 3 Eric Seidel (no email) 2010-05-13 11:13:09 PDT
It's just like how we dynamically skip tests on Windows when your machine isn't supported for hardware compositing.

This came up because the Tiger bot currently has Python 2.3, and the resolution to that was to skip all the websocket tests (which is bad).  I'd rather have the skip only happen if someone is missing the proper python.  We can display a huge warning if you like.

I'm happy to close this as WONTFIX, as it means less work for me.  But I think automatically skipping with a big warning is a better behavior than our current manual skip or than making it impossible to run test at all w/o python 2.5.
Comment 4 Alexey Proskuryakov 2010-05-13 11:20:22 PDT
> It's just like how we dynamically skip tests on Windows when your machine
> isn't supported for hardware compositing.

Yes, but Tiger support WebSocket just fine. It's purely a tools issue, unlike the compositing one.
Comment 5 Fumitoshi Ukai 2010-05-17 00:56:05 PDT
Created attachment 56219 [details]
Patch
Comment 6 Shinichiro Hamaji 2010-06-02 23:33:25 PDT
Comment on attachment 56219 [details]
Patch

Hi Alexey, this patch would not be the best solution, but seems to make the situation better. If I understand correctly, now we are just skipping all websocket tests by mac-tiger/Skipped even when python 2.5 is manually installed. So, I'd like to put r+ for this patch. Or, we might want to add a flag like --skip-python25-tests-if-we-dont-have-python25 into old-run-webkit-tests and use this flag in our buildbot for now. What do you think?

By the way Ukai-san, cannot we remove websocket/tests from mac-tiger/Skipped with this patch?
Comment 7 Fumitoshi Ukai 2010-06-02 23:58:54 PDT
(In reply to comment #6)

> By the way Ukai-san, cannot we remove websocket/tests from mac-tiger/Skipped with this patch?

yes, we can.
Comment 8 Fumitoshi Ukai 2010-06-02 23:59:33 PDT
Created attachment 57738 [details]
Patch
Comment 9 Alexey Proskuryakov 2010-06-03 10:19:31 PDT
Comment on attachment 57738 [details]
Patch

>         print "Your Python version is insuficient to run WebKit's Python code.  Please update.\n";

Typo: should be "insufficient".

+    `./WebKitTools/Scripts/ensure-valid-python --check-only`;

We normally use system(), not backticks. But there's precedent for the latter, too.
Comment 10 Fumitoshi Ukai 2010-06-03 21:22:24 PDT
Committed r60652: <http://trac.webkit.org/changeset/60652>
Comment 11 Eric Seidel (no email) 2010-06-07 09:45:15 PDT
Can't exec "WebKitTools/Scripts/ensure-valid-python": No such file or directory at ../../Projects/WebKit/WebKitTools/Scripts/old-run-webkit-tests line 1441.
WARNING: Your platform does not have Python 2.5+, which is required to run websocket server, so disabling websocket/tests.
Running build-dumprendertree

I get that when trying to call run-webkit-tests from outside of a webkit checkout.  Seems the change didn't use the wkdirs module as it should have.
Comment 12 Eric Seidel (no email) 2010-06-07 09:49:17 PDT
Yes, this is the wrong way to launch scripts:

 1441     `./WebKitTools/Scripts/ensure-valid-python --check-only`;

The perl code is lame about how it handles paths.  The python code always uses absolute paths.  The other callsites make similar relative-path calls and get away with it because they're after the chdirWebKit() call in old-run-webkit-tests.

I think you should instead make your path absolute using sourceDir() from webkitdirs.  You could also use relativeScriptsDir().
Comment 13 Eric Seidel (no email) 2010-06-08 02:14:55 PDT
Can't exec "WebKitTools/Scripts/ensure-valid-python": No such file or directory at ../WebKitTools/Scripts/old-run-webkit-tests line 1441.
WARNING: Your platform does not have Python 2.5+, which is required to run websocket server, so disabling websocket/tests.

I'm getting this all the time.  We need to fix or roll this out (please). :)
Comment 14 Fumitoshi Ukai 2010-06-08 02:41:59 PDT
Created attachment 58125 [details]
Patch
Comment 15 Eric Seidel (no email) 2010-06-08 10:53:34 PDT
Comment on attachment 58125 [details]
Patch

I think its good practice for all paths to be absolute, regardless of what the current directory is.  I wouldn't bother adding the comment, but this looks fine.
Comment 16 Eric Seidel (no email) 2010-06-08 10:54:01 PDT
Comment on attachment 57738 [details]
Patch

Marking this patch obsolete since it was landed.
Comment 17 Fumitoshi Ukai 2010-06-08 18:03:03 PDT
Committed r60875: <http://trac.webkit.org/changeset/60875>