WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
74138
new-run-webkit-tests: clean up host/systemhost interactions in the port subsystem
https://bugs.webkit.org/show_bug.cgi?id=74138
Summary
new-run-webkit-tests: clean up host/systemhost interactions in the port subsy...
Dirk Pranke
Reported
2011-12-08 16:23:03 PST
new-run-webkit-tests: clean up host/systemhost interactions in the port subsystem
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
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Dirk Pranke
Comment 1
2011-12-08 16:38:56 PST
Created
attachment 118488
[details]
Patch
Dirk Pranke
Comment 2
2011-12-08 16:59:02 PST
Created
attachment 118492
[details]
merge to
r102395
Eric Seidel (no email)
Comment 3
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. :)
Dirk Pranke
Comment 4
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.
Dirk Pranke
Comment 5
2011-12-13 15:53:33 PST
"blip" ... "blip" ... (keeping it on the radar)
David Levin
Comment 6
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.
Dirk Pranke
Comment 7
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.
Adam Barth
Comment 8
2011-12-13 17:31:42 PST
(I can take a look this evening or tomorrow morning.)
Eric Seidel (no email)
Comment 9
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.
Eric Seidel (no email)
Comment 10
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. :)
Dirk Pranke
Comment 11
2011-12-13 18:28:16 PST
Okay, I will break this up into chunks and cancel the r? on this.
Dirk Pranke
Comment 12
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!
Dirk Pranke
Comment 13
2011-12-21 13:56:22 PST
closing ... all of the individual patches have landed.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug