WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
208485
Add Unittest to commits_for_uploads()
https://bugs.webkit.org/show_bug.cgi?id=208485
Summary
Add Unittest to commits_for_uploads()
Matt Lewis
Reported
2020-03-02 16:30:07 PST
While working on commits_for_upload() I found multiple issues due to the fact that it had no unit testing.
Attachments
Patch
(12.09 KB, patch)
2020-03-02 17:17 PST
,
Matt Lewis
no flags
Details
Formatted Diff
Diff
Patch
(12.28 KB, patch)
2020-03-03 12:20 PST
,
Matt Lewis
jbedard
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2020-03-02 16:30:20 PST
<
rdar://problem/59973902
>
Matt Lewis
Comment 2
2020-03-02 17:17:36 PST
Created
attachment 392228
[details]
Patch Uploading initial patch to test on both git and svn style checkouts.
Matt Lewis
Comment 3
2020-03-02 17:36:02 PST
Comment on
attachment 392228
[details]
Patch Tested run-webkit-test with all permutations of checkouts that we expect.
Jonathan Bedard
Comment 4
2020-03-02 17:48:39 PST
Comment on
attachment 392228
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=392228&action=review
> Tools/ChangeLog:26 > + (PortTest.make_port): changed from MockSystemHost to MockHost
This deserves a 'why'. You mentioned in person the rationale, you should call that out here.
> Tools/Scripts/webkitpy/common/checkout/scm/stub_repository.py:43 > self._filesystem = filesystem
Can you check if the SCM constructor does this already? Feels like it should. If it doesn't lets leave this code as is.
> Tools/Scripts/webkitpy/common/checkout/scm/stub_repository_unittest.py:46 > + def mock_executive_for_stub_repository():
This function is a bit weird unless you intend to expand it later and add mock command results. My general rule is if you find yourself defining more than one of the host member objects, you should return a host object. Here, I think we should either go back to the mock host or just pass a constructed executive inline to each calcite (and import MockExecutive instead of executive_mock)
Matt Lewis
Comment 5
2020-03-03 12:20:10 PST
Created
attachment 392308
[details]
Patch This should address the various comments that were given.
Matt Lewis
Comment 6
2020-03-03 13:08:55 PST
Committed
r257796
: <
https://trac.webkit.org/changeset/257796
>
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