Bug 90513 – nrwt: reimplement manager_worker_broker in a much simpler form

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

nrwt: reimplement manager_worker_broker in a much simpler form

[Dirk Pranke]

Created attachment 150711 [details]
Patch

[Dirk Pranke]

Created attachment 151830 [details]
merge to r122394

[Ojan Vafai]

Comment on attachment 151830 [details]
merge to r122394

View in context: https://bugs.webkit.org/attachment.cgi?id=151830&action=review

That's a lot of deleted code!

> Tools/ChangeLog:19
> +        I'm removing manager_worker_broker_unittest.py as well; we get
> +        nearly complete coverage from the integration tests, and will
> +        get more coverage when test-webkitpy moves to use this as well,
> +        so having unit tests seems like unnecessary overhead. (running
> +        coverage numbers with test-webkitpy shows that pretty much the only
> +        uncovered lines are lines that are only run in the child processes,
> +        which coverage doesn't handle at the moment).

I'm a little torn about this. I think it makes it harder to make changes to this code in the future and have confidence. On the other hand, I believe you that integration tests cover most of the code paths.

abarth, wdyt?

> Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:98
> +            worker.running_inline = self._running_inline

Should running_inline and manager just be arguments to the constructor? There doesn't seem to be benefit to setting those after the constructor.

[Adam Barth]

> abarth, wdyt?

IMHO, it's fine.  We run the integration tests by default, so we're not missing out on too much test coverage.

[Dirk Pranke]

(In reply to comment #3)
> > Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:98
> > +            worker.running_inline = self._running_inline
> 
> Should running_inline and manager just be arguments to the constructor? There doesn't seem to be benefit to setting those after the constructor.

Sure, I can make that change.

[Dirk Pranke]

Committed r122497: <http://trac.webkit.org/changeset/122497>