Bug 208485 - Add Unittest to commits_for_uploads()
Summary: Add Unittest to commits_for_uploads()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Matt Lewis
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-03-02 16:30 PST by Matt Lewis
Modified: 2020-03-03 13:08 PST (History)
4 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Lewis 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.
Comment 1 Radar WebKit Bug Importer 2020-03-02 16:30:20 PST
<rdar://problem/59973902>
Comment 2 Matt Lewis 2020-03-02 17:17:36 PST
Created attachment 392228 [details]
Patch

Uploading initial patch to test on both git and svn style checkouts.
Comment 3 Matt Lewis 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.
Comment 4 Jonathan Bedard 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)
Comment 5 Matt Lewis 2020-03-03 12:20:10 PST
Created attachment 392308 [details]
Patch

This should address the various comments that were given.
Comment 6 Matt Lewis 2020-03-03 13:08:55 PST
Committed r257796: <https://trac.webkit.org/changeset/257796>