Bug 35416 - new-run-webkit-tests: split Port.check_sys_deps() into two calls
Summary: new-run-webkit-tests: split Port.check_sys_deps() into two calls
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: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-25 18:51 PST by Dirk Pranke
Modified: 2010-03-01 12:41 PST (History)
5 users (show)

See Also:


Attachments
Patch (21.51 KB, patch)
2010-02-25 18:53 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
patch revised w/ ojan's feedback: removed --nocheck-build and --nostart-helper flags (21.50 KB, patch)
2010-02-26 15:46 PST, Dirk Pranke
levin: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2010-02-25 18:51:23 PST
Currently there is a circular dependency between Port.check_sys_deps() and Port.start_helper() - you need to start the helper before check_sys_deps() will pass on chromium windows, but that gives you no way to check if the helper has been built (since that would be done in check_sys_deps() ).
Comment 1 Dirk Pranke 2010-02-25 18:53:02 PST
Created attachment 49559 [details]
Patch
Comment 2 Ojan Vafai 2010-02-26 15:22:32 PST
Comment on attachment 49559 [details]
Patch

Just a couple nits.

> +++ b/WebKitTools/ChangeLog
> +        Add Port.check_build() call that is separate from Port.check_sys_deps()
> +        (and add a --nocheck-build flag to skip).

Do we need a --nocheck-build flag? Unlike checking system dependencies, if we can't find DRT/TestShell, then this script really can't do anything useful.

> +++ b/WebKitTools/Scripts/webkitpy/layout_tests/port/base.py
>      def start_helper(self):
> -        """Start a layout test helper if needed on this port. The test helper
> -        is used to reconfigure graphics settings and do other things that
> -        may be necessary to ensure a known test configuration."""
> -        raise NotImplementedError('Port.start_helper')
> +        """If a port needs to reconfigure graphics settings or do other
> +        things to ensure a known test configuration, it should override this
> +        method. The helper can be skipped with --nostart-helper."""
> +        pass

> +++ b/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py
> @@ -42,10 +42,14 @@ import http_server
>  import websocket_server
>  
>  
> -def check_file_exists(path_to_file, str):
> +def check_file_exists(path_to_file, exe_str, override_str=None):
Documenting these arguments would help. I'm not really sure what exe_str and override_str are.Now that I look at how it's used, names like "exe_description" and "override_description" might be a bit more clear.

> @@ -1621,6 +1630,9 @@ def parse_args(args=None):
> +    option_parser.add_option("", "--nocheck-build",
> +                             action="store_true", default=False,
> +                             help="don't check that the build is up to date")
>      option_parser.add_option("", "--nostart-helper",
>                               action="store_true", default=False,
>                               help="don't run layout_test_helper")

I know you're not adding it in this change, but do we need --nostart-helper? When would you ever want to run the tests without the start helper?
Comment 3 Dirk Pranke 2010-02-26 15:26:57 PST
(In reply to comment #2)

I could see that maybe --nocheck-build and --nostart-helper are too esoteric (they are useful if you want to run the 'passing-*' platforms, which stub out TestShell and don't need the layout_test_helper.

I will update the arg names.
Comment 4 Dirk Pranke 2010-02-26 15:46:09 PST
Created attachment 49657 [details]
patch revised w/ ojan's feedback: removed --nocheck-build and --nostart-helper flags
Comment 5 Ojan Vafai 2010-02-26 17:09:40 PST
Comment on attachment 49657 [details]
patch revised w/ ojan's feedback: removed --nocheck-build and --nostart-helper flags

Please update the changelog description. Otherwise, LGTM.
Comment 6 WebKit Commit Bot 2010-02-26 20:26:34 PST
Comment on attachment 49657 [details]
patch revised w/ ojan's feedback: removed --nocheck-build and --nostart-helper flags

Rejecting patch 49657 from commit-queue.

Failed to run "['git', 'svn', 'dcommit']" exit_code: 1
Last 500 characters of output:
M	WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py
	M	WebKitTools/Scripts/webkitpy/thirdparty/autoinstall.py
A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:
svnlook: Can't write to stream: Broken pipe

    The following ChangeLog files contain OOPS:

        trunk/WebKitTools/ChangeLog

    Please don't ever say "OOPS" in a ChangeLog file.
 at /usr/local/git/libexec/git-core/git-svn line 558


Full output: http://webkit-commit-queue.appspot.com/results/314824
Comment 7 David Levin 2010-03-01 11:32:08 PST
Comment on attachment 49657 [details]
patch revised w/ ojan's feedback: removed --nocheck-build and --nostart-helper flags

If possible avoid the comparisons to 0 (several places have "!= 0").
Comment 8 Dirk Pranke 2010-03-01 12:41:38 PST
Committed r55373: <http://trac.webkit.org/changeset/55373>