Bug 35416 – new-run-webkit-tests: split Port.check_sys_deps() into two calls

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Dirk Pranke]

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() ).

[Dirk Pranke]

Created attachment 49559 [details]
Patch

[Ojan Vafai]

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?

[Dirk Pranke]

(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.

[Dirk Pranke]

Created attachment 49657 [details]
patch revised w/ ojan's feedback: removed --nocheck-build and --nostart-helper flags

[Ojan Vafai]

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.

[WebKit Commit Bot]

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

[David Levin]

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").

[Dirk Pranke]

Committed r55373: <http://trac.webkit.org/changeset/55373>