nrwt: reimplement manager_worker_broker in a much simpler form
Created attachment 150711 [details]
Created attachment 151830 [details]
merge to r122394
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!
> + 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.
> + 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.
> abarth, wdyt?
IMHO, it's fine. We run the integration tests by default, so we're not missing out on too much test coverage.
(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.
Committed r122497: <http://trac.webkit.org/changeset/122497>