Bug 44902 – new-run-webkit-tests: add more unit tests

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dirk Pranke]

new-run-webkit-tests: add more unit tests

[Dirk Pranke]

Created attachment 65966 [details]
Patch

[Ojan Vafai]

Comment on attachment 65966 [details]
Patch

> :140
> +    hdlr = logging.StreamHandler(meter)
s/hdlr/handler

Here and in many places below and in other files.

[Dirk Pranke]

Committed r66542: <http://trac.webkit.org/changeset/66542>

[Dirk Pranke]

Created attachment 66151 [details]
Patch

[Dirk Pranke]

the first patch was rolled out in r66547.

[Tony Chang]

Comment on attachment 66151 [details]
Patch

> +    def cleanup(self):
> +        """Restore logging configuration to its initial settings."""
> +        _restore_logging(self._old_handler)
> +        self._old_handler = None

Should there be a 'if self._old_handler:' here like in the __del__ method below?

Sorry, I have to run.  I can finish the review in a few hours.

[Dirk Pranke]

(In reply to comment #6)
> (From update of attachment 66151 [details])
> > +    def cleanup(self):
> > +        """Restore logging configuration to its initial settings."""
> > +        _restore_logging(self._old_handler)
> > +        self._old_handler = None
> 
> Should there be a 'if self._old_handler:' here like in the __del__ method below?
> 

I suppose it's possible to (improperly) call cleanup() several times in a row, in which case an if: would be a good idea. I can add that. Alternatively, I could get rid of the if: in the __del__ and trust the caller to do the right thing, since this is a fairly private and specialized class. The if in the __del__ is only there in case the user forgot to call cleanup(), but it is precarious functionality to rely on, since we don't have a lot of control over when the __del__ is invoked.

[Tony Chang]

Comment on attachment 66151 [details]
Patch

> +    def cleanup(self):
> +        """Restore logging configuration to its initial settings."""
> +        _restore_logging(self._old_handler)
> +        self._old_handler = None
> +
> +    def __del__(self):
> +        if self._old_handler:
> +            _restore_logging(self._old_handler)

Maybe __del__ should just call cleanup?  I don't feel strongly about this.

> diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py b/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py

> +    try:
> +        # Configure the printing subsystem for printing output, logging debug
> +        # info, and tracing tests.

As discussed on IRC, in a follow up change, let's try to break this method into smaller methods so we can reduce the scope of the try/finally block.

> +        if not options.child_processes:
> +            # FIXME: Investigate perf/flakiness impact of using cpu_count + 1.
> +            options.child_processes = port_obj.default_child_processes()

Can we move this to before the try/finally?

> +        printer = printing.Printer(port_obj, options, regular_output=regular_output,
> +            buildbot_output=buildbot_output,
> +            child_processes=int(options.child_processes),
> +            is_fully_parallel=options.experimental_fully_parallel)

Can we move this before the try too?  Then in the finally you don't have to check if printer is None.

> +        if not options.configuration:
> +            options.configuration = port_obj.default_configuration()
> +
> +        if options.pixel_tests is None:
> +            options.pixel_tests = True
> +
> +        if not options.use_apache:
> +            options.use_apache = sys.platform in ('darwin', 'linux2')
> +
> +        if options.results_directory.startswith("/"):
> +            # Assume it's an absolute path and normalize.
> +            options.results_directory = port_obj.get_absolute_path(
> +                options.results_directory)
> +        else:
> +            # If it's a relative path, make the output directory relative to
> +            # Debug or Release.
> +            options.results_directory = port_obj.results_directory()

You could move all this options stuff to before the try too.

> +    finally:
> +        port_obj.stop_helper()
> +        if test_runner:
> +            test_runner.cleanup()
> +        _log.debug("Exit status: %d" % num_unexpected_results)

When this method does a return 0, won't this output -1?  That seems confusing.


Perhaps I have made enough code restructuring suggestions for run() that you can refactor that in a separate change before this one?  Anyway, your call.

[Dirk Pranke]

Created attachment 66305 [details]
Patch

[Dirk Pranke]

(In reply to comment #8)
> (From update of attachment 66151 [details])
> > +    def cleanup(self):
> > +        """Restore logging configuration to its initial settings."""
> > +        _restore_logging(self._old_handler)
> > +        self._old_handler = None
> > +
> > +    def __del__(self):
> > +        if self._old_handler:
> > +            _restore_logging(self._old_handler)
> 
> Maybe __del__ should just call cleanup?  I don't feel strongly about this.
> 
> > diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py b/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py
> 
> > +    try:
> > +        # Configure the printing subsystem for printing output, logging debug
> > +        # info, and tracing tests.
> 
> As discussed on IRC, in a follow up change, let's try to break this method into smaller methods so we can reduce the scope of the try/finally block.
> 
> > +        if not options.child_processes:
> > +            # FIXME: Investigate perf/flakiness impact of using cpu_count + 1.
> > +            options.child_processes = port_obj.default_child_processes()
> 
> Can we move this to before the try/finally?
> 
> > +        printer = printing.Printer(port_obj, options, regular_output=regular_output,
> > +            buildbot_output=buildbot_output,
> > +            child_processes=int(options.child_processes),
> > +            is_fully_parallel=options.experimental_fully_parallel)
> 
> Can we move this before the try too?  Then in the finally you don't have to check if printer is None.
> 
> > +        if not options.configuration:
> > +            options.configuration = port_obj.default_configuration()
> > +
> > +        if options.pixel_tests is None:
> > +            options.pixel_tests = True
> > +
> > +        if not options.use_apache:
> > +            options.use_apache = sys.platform in ('darwin', 'linux2')
> > +
> > +        if options.results_directory.startswith("/"):
> > +            # Assume it's an absolute path and normalize.
> > +            options.results_directory = port_obj.get_absolute_path(
> > +                options.results_directory)
> > +        else:
> > +            # If it's a relative path, make the output directory relative to
> > +            # Debug or Release.
> > +            options.results_directory = port_obj.results_directory()
> 
> You could move all this options stuff to before the try too.
> 
> > +    finally:
> > +        port_obj.stop_helper()
> > +        if test_runner:
> > +            test_runner.cleanup()
> > +        _log.debug("Exit status: %d" % num_unexpected_results)
> 
> When this method does a return 0, won't this output -1?  That seems confusing.
> 

Good catch. Yes, that was a bug.

I ended up doing all of the restructuring and rolling it into this patch (sorry!); I also re-jiggered the logging handler setup/teardown in printing.py to not attempt to be smart about keeping only one handler active; that was very fragile and buggy.

The good news about all of the refactoring, though - it's all covered by unit tests, so it's pretty low-risk :)

[Tony Chang]

Comment on attachment 66305 [details]
Patch

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!

[Dirk Pranke]

(In reply to comment #11)
> (From update of attachment 66305 [details])
> 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.
> 

Good suggestions. Done.

> > @@ -714,6 +721,43 @@ class TestRunner:
> > +    def set_up_run(self):
> 
> Nit: Document this function and the return.
>

Done.
 
> > +    # 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.
>

Fixed.
 
> 
> Yay for lots of new tests!

[Dirk Pranke]

Committed r66640: <http://trac.webkit.org/changeset/66640>