Bug 74138 – new-run-webkit-tests: clean up host/systemhost interactions in the port subsystem

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

new-run-webkit-tests: clean up host/systemhost interactions in the port subsystem

[Dirk Pranke]

Created attachment 118488 [details]
Patch

[Dirk Pranke]

Created attachment 118492 [details]
merge to r102395

[Eric Seidel (no email)]

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. :)

[Dirk Pranke]

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.

[Dirk Pranke]

"blip" ... "blip" ... (keeping it on the radar)

[David Levin]

(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.

[Dirk Pranke]

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.

[Adam Barth]

(I can take a look this evening or tomorrow morning.)

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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. :)

[Dirk Pranke]

Okay, I will break this up into chunks and cancel the r? on this.

[Dirk Pranke]

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!

[Dirk Pranke]

closing ... all of the individual patches have landed.