| Summary: | nrwt: reimplement manager_worker_broker in a much simpler form | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Dirk Pranke <dpranke> | ||||||
| Component: | New Bugs | Assignee: | Dirk Pranke <dpranke> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | abarth, eric, ojan, tony | ||||||
| Priority: | P2 | Keywords: | NRWT | ||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 89725 | ||||||||
| Attachments: |
|
||||||||
|
Description
Dirk Pranke
2012-07-03 19:41:35 PDT
Created attachment 150711 [details]
Patch
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! > 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. > 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> |