Bug 74138 - new-run-webkit-tests: clean up host/systemhost interactions in the port subsystem
Summary: new-run-webkit-tests: clean up host/systemhost interactions in the port subsy...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on: 74551 74556 74562 74566 74878
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-08 16:23 PST by Dirk Pranke
Modified: 2011-12-21 13:56 PST (History)
6 users (show)

See Also:


Attachments
Patch (88.52 KB, patch)
2011-12-08 16:38 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
merge to r102395 (88.64 KB, patch)
2011-12-08 16:59 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2011-12-08 16:23:03 PST
new-run-webkit-tests: clean up host/systemhost interactions in the port subsystem
Comment 1 Dirk Pranke 2011-12-08 16:38:56 PST
Created attachment 118488 [details]
Patch
Comment 2 Dirk Pranke 2011-12-08 16:59:02 PST
Created attachment 118492 [details]
merge to r102395
Comment 3 Eric Seidel (no email) 2011-12-08 17:08:28 PST
Comment on attachment 118488 [details]
Patch

I like the change.  It's a bit difficult to take in at this size, but I'll just come back for another reading. :)
Comment 4 Dirk Pranke 2011-12-08 17:21:15 PST
Yeah, unfortunately it was kind of an all-or-nothing thing in order to get all the tests to pass. I changed as little as I could while making the fundamental change to making the host parameter to Port() required :(. 

Take your time ... there's no real urgency on this.
Comment 5 Dirk Pranke 2011-12-13 15:53:33 PST
"blip" ... "blip" ... (keeping it on the radar)
Comment 6 David Levin 2011-12-13 16:14:11 PST
(In reply to comment #4)
> Yeah, unfortunately it was kind of an all-or-nothing thing in order to get all the tests to pass. I changed as little as I could while making the fundamental change to making the host parameter to Port() required :(. 

Why not do patches leading up to making the host parameter to Port required?

You could use host= at the calling sites and making it the first parameter (which seems possible because all parameters are optional now). 

The final patch would make the parameter required and remove host= in calling sites.
Comment 7 Dirk Pranke 2011-12-13 16:20:10 PST
okay, admittedly I could probably break this up into a few smaller patches, but I was hoping to not have to spend the time to do so. I believe Eric has looked at this a little bit, so I'll let him say if he thinks this is not unreasonable to review as is, or if he'd like me to break it up. 

The actual patch isn't very complicated; it's mostly just search&replace with a few other spots that have actual changes as noted in the ChangeLogs.
Comment 8 Adam Barth 2011-12-13 17:31:42 PST
(I can take a look this evening or tomorrow morning.)
Comment 9 Eric Seidel (no email) 2011-12-13 18:06:10 PST
I've looked at it in part a couple times today.  But never made it all the way through.

I agree with Levin, this could be done in pieces.  That certainly would make this a quicker review.
Comment 10 Eric Seidel (no email) 2011-12-13 18:07:03 PST
Comment on attachment 118492 [details]
merge to r102395

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

> Tools/ChangeLog:13
> +        2) Unit tests for the Port objects can use a full MockHost

Seems slightly dangerous if in practice Port() can only be expected to have a SystemHost.  Is this just a partial step along that goal?

> Tools/ChangeLog:18
> +        3) the TestPort object no longer modifies the (mock) filesystem at
> +           all; clients are required to call
> +           add_unit_tests_to_mock_filesystem() to ensure that the system is set
> +           up correctly. The MockHost() object takes care of this for
> +           most callers.

This is a fantastic change and could be done first. :)
Comment 11 Dirk Pranke 2011-12-13 18:28:16 PST
Okay, I will break this up into chunks and cancel the r? on this.
Comment 12 Dirk Pranke 2011-12-14 17:26:42 PST
I was able to break this patch into 4 separate things; they're listed as blockers for this now. Please take a look when you get a chance. Thanks!
Comment 13 Dirk Pranke 2011-12-21 13:56:22 PST
closing ... all of the individual patches have landed.