[webkit-reviews] review granted: [Bug 44902] new-run-webkit-tests: add more unit tests : [Attachment 66305] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has granted Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 44902: new-run-webkit-tests: add more unit tests
https://bugs.webkit.org/show_bug.cgi?id=44902

Attachment 66305: Patch
https://bugs.webkit.org/attachment.cgi?id=66305&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
This looks a lot better! Thanks for refactoring.

> diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py
b/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py
> +	   new_args = []
> +	   for arg in args:
> +	       if arg and arg != '':
> +		   new_args.append(arg)
> +	   paths = new_args

Sorry, I know you're just moving this, but why not just:
paths = [arg for arg in args if arg and arg != '']

> +	   if not paths:
> +	       paths = []

Nit: This check doesn't seem necessary.

> @@ -714,6 +721,43 @@ class TestRunner:
> +    def set_up_run(self):

Nit: Document this function and the return.

> +    # We wrap any parts of the run that are slow or likely to raise
exceptions
> +    # in a try/finally to ensure that we clean up the logging configuration.

> +    num_unexpected_results = -1
> +    try:
> +	   test_runner = TestRunner(port_obj, options, printer)
> +	   test_runner._print_config()
> +
> +	   printer.print_update("Collecting tests ...")
> +	   test_runner.collect_tests(args, last_unexpected_results)
> +
> +	   printer.print_update("Parsing expectations ...")
> +	   if options.lint_test_files:
> +	       return test_runner.lint()
> +	   test_runner.parse_expectations(port_obj.test_platform_name(),
> +				       options.configuration == 'Debug')

Nit: Indention looks funny here.


Yay for lots of new tests!