Bug 35416 - new-run-webkit-tests: split Port.check_sys_deps() into two calls
: new-run-webkit-tests: split Port.check_sys_deps() into two calls
Status: RESOLVED FIXED
: WebKit
Tools / Tests
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-02-25 18:51 PST by
Modified: 2010-03-01 12:41 PST (History)


Attachments
Patch (21.51 KB, patch)
2010-02-25 18:53 PST, Dirk Pranke
no flags Review Patch | 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-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2010-02-25 18:53:02 PST -------
Created an attachment (id=49559) [details]
Patch
------- Comment #2 From 2010-02-26 15:22:32 PST -------
(From update of attachment 49559 [details])
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 From 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 From 2010-02-26 15:46:09 PST -------
Created an attachment (id=49657) [details]
patch revised w/ ojan's feedback: removed --nocheck-build and --nostart-helper flags
------- Comment #5 From 2010-02-26 17:09:40 PST -------
(From update of attachment 49657 [details])
Please update the changelog description. Otherwise, LGTM.
------- Comment #6 From 2010-02-26 20:26:34 PST -------
(From update of attachment 49657 [details])
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 From 2010-03-01 11:32:08 PST -------
(From update of attachment 49657 [details])
If possible avoid the comparisons to 0 (several places have "!= 0").
------- Comment #8 From 2010-03-01 12:41:38 PST -------
Committed r55373: <http://trac.webkit.org/changeset/55373>