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() ).
Created attachment 49559 [details] Patch
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?
(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.
Created attachment 49657 [details] patch revised w/ ojan's feedback: removed --nocheck-build and --nostart-helper flags
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 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 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").
Committed r55373: <http://trac.webkit.org/changeset/55373>