Bug 200787 - run-webkit-test: Allow results to be uploaded without scm checkout
Summary: run-webkit-test: Allow results to be uploaded without scm checkout
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: 2019-08-15 13:40 PDT by Matt Lewis
Modified: 2019-09-10 18:12 PDT (History)
7 users (show)

See Also:


Attachments
Patch (2.19 KB, patch)
2019-08-15 15:44 PDT, Matt Lewis
no flags Details | Formatted Diff | Diff
Patch (8.31 KB, patch)
2019-08-20 15:38 PDT, Matt Lewis
no flags Details | Formatted Diff | Diff
Patch (9.07 KB, patch)
2019-08-21 17:43 PDT, Matt Lewis
no flags Details | Formatted Diff | Diff
Patch (7.91 KB, patch)
2019-08-23 11:40 PDT, Matt Lewis
no flags Details | Formatted Diff | Diff
Patch (7.89 KB, patch)
2019-08-26 16:22 PDT, Matt Lewis
no flags Details | Formatted Diff | Diff
Patch (12.11 KB, patch)
2019-08-29 08:43 PDT, Matt Lewis
no flags Details | Formatted Diff | Diff
Patch (12.27 KB, patch)
2019-09-05 18:33 PDT, Matt Lewis
no flags Details | Formatted Diff | Diff
Patch (12.51 KB, patch)
2019-09-06 09:17 PDT, Matt Lewis
no flags Details | Formatted Diff | Diff
Patch (12.59 KB, patch)
2019-09-06 09:31 PDT, Matt Lewis
no flags Details | Formatted Diff | Diff
Patch (12.44 KB, patch)
2019-09-06 09:48 PDT, Matt Lewis
no flags Details | Formatted Diff | Diff
Patch (12.54 KB, patch)
2019-09-10 13:39 PDT, Matt Lewis
no flags Details | Formatted Diff | Diff
Patch (13.24 KB, patch)
2019-09-10 14:19 PDT, Matt Lewis
no flags Details | Formatted Diff | Diff
Patch (13.26 KB, patch)
2019-09-10 15:22 PDT, Matt Lewis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Lewis 2019-08-15 13:40:37 PDT
run-webkit-test: Allow results to be uploaded without scm checkout
Comment 1 Radar WebKit Bug Importer 2019-08-15 15:06:00 PDT
<rdar://problem/54364258>
Comment 2 Matt Lewis 2019-08-15 15:44:48 PDT
Created attachment 376435 [details]
Patch
Comment 3 Alexey Proskuryakov 2019-08-15 16:23:56 PDT
Comment on attachment 376435 [details]
Patch

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

> Tools/Scripts/webkitpy/port/base.py:1617
> +            if os.path.exists(os.path.join(path, 'repo.json')):

This is an abbreviation, and not a very descriptive name. Would “checkout-information.json” be accurate?
Comment 4 Matt Lewis 2019-08-15 16:27:30 PDT
(In reply to Alexey Proskuryakov from comment #3)
> Comment on attachment 376435 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=376435&action=review
> 
> > Tools/Scripts/webkitpy/port/base.py:1617
> > +            if os.path.exists(os.path.join(path, 'repo.json')):
> 
> This is an abbreviation, and not a very descriptive name. Would
> “checkout-information.json” be accurate?

That is probably far more appropriate.
Comment 5 Jonathan Bedard 2019-08-15 16:58:15 PDT
Comment on attachment 376435 [details]
Patch

Turns out, this won't work because there are two places we do this. I talked to Matt about moving this json file check into the detect_scm_system codepath, and I think that's what we're going to need to do.
Comment 6 Matt Lewis 2019-08-20 15:38:22 PDT
Created attachment 376821 [details]
Patch
Comment 7 Jonathan Bedard 2019-08-20 15:55:05 PDT
Comment on attachment 376821 [details]
Patch

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

> Tools/Scripts/webkitpy/common/checkout/scm/stub_repository.py:83
> +        return json_data['id']

This should probably call native_revision(...)
Comment 8 Matt Lewis 2019-08-21 17:43:42 PDT
Created attachment 376956 [details]
Patch

Work towards fixing a json and scm detection error
Comment 9 Matt Lewis 2019-08-23 11:40:23 PDT
Created attachment 377141 [details]
Patch

Work toward consistently running
Comment 10 Matt Lewis 2019-08-26 16:22:33 PDT
Created attachment 377284 [details]
Patch
Comment 11 Matt Lewis 2019-08-26 16:23:32 PDT
Tested the changes with the stub repository and regular checkouts and worked locally.
Comment 12 Jonathan Bedard 2019-08-26 16:37:41 PDT
Comment on attachment 377284 [details]
Patch

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

> Tools/Scripts/webkitpy/common/checkout/scm/stub_repository.py:61
> +    def in_working_directory(cls, path, executive=None):

What do the Git/SVN classes say if you give them something like OpenSource/Tools? Does 'in_working_directory' return true or false?
Comment 13 Matt Lewis 2019-08-26 17:02:41 PDT
(In reply to Jonathan Bedard from comment #12)
> Comment on attachment 377284 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377284&action=review
> 
> > Tools/Scripts/webkitpy/common/checkout/scm/stub_repository.py:61
> > +    def in_working_directory(cls, path, executive=None):
> 
> What do the Git/SVN classes say if you give them something like
> OpenSource/Tools? Does 'in_working_directory' return true or false?

Well based on the classes they return True if the .svn directory is in the path or they run the svn and/or git commands and look at the output to find out if they are in a repo. Otherwise they catch an exception out and return False.
Comment 14 Matt Lewis 2019-08-26 17:03:13 PDT
(In reply to Matt Lewis from comment #13)
> (In reply to Jonathan Bedard from comment #12)
> > Comment on attachment 377284 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=377284&action=review
> > 
> > > Tools/Scripts/webkitpy/common/checkout/scm/stub_repository.py:61
> > > +    def in_working_directory(cls, path, executive=None):
> > 
> > What do the Git/SVN classes say if you give them something like
> > OpenSource/Tools? Does 'in_working_directory' return true or false?
> 
> Well based on the classes they return True if the .svn directory is in the
> path or they run the svn and/or git commands and look at the output to find
> out if they are in a repo. Otherwise they catch an exception out and return
> False.

Based on my understanding of the classes*
Comment 15 Matt Lewis 2019-08-29 08:43:21 PDT
Created attachment 377593 [details]
Patch

Added unittesting for stub repository.
Comment 16 Jonathan Bedard 2019-08-29 09:14:08 PDT
Comment on attachment 377593 [details]
Patch

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

> Tools/Scripts/webkitpy/common/checkout/scm/stub_repository.py:38
> +from .scm import AuthenticationError, SCM, commit_error_handler

We don't need AuthenticationError or commit_error_handler

> Tools/Scripts/webkitpy/common/checkout/scm/stub_repository.py:43
> +class StubRepository(SCM):

High level comment: Since every other SCM's in_worker_directory works recursively, this one should to. You could just duplicate the loop logic, but something like this might be more elegant:

@classmethod
def _run_at_root(cls, path, callback, filesystem=None):
    if not filesystem:
        filesystem = SystemHost().filesystem

    while path:
        if filesystem.isfile(filesystem.join(path, 'checkout_information.json')):
            return callback(path, filesystem)
        path = filesystem.dirname(path)

    return None

Then, your functions would look like this:

@classmethod
def in_working_directory(cls, path, executive=None, filesystem=None):
    return bool(cls._run_at_root(path, lambda path, filesystem: True, filesystem=filesystem))

...

def svn_revision(self, path):
    return self._run_at_root(path, lambda path, filesystem: self._decode_json(path)['id'], filesystem=self._filesystem)

> Tools/Scripts/webkitpy/common/checkout/scm/stub_repository.py:49
> +        if filesystem.isfile(filesystem.join(path, 'checkout_information.json')):

checkout_information.json should be a class variable

> Tools/Scripts/webkitpy/common/checkout/scm/stub_repository.py:70
> +        # self.native_revision(path)

What is this comment doing?

> Tools/Scripts/webkitpy/common/checkout/scm/stub_repository_unittest.py:41
> +        # stub_repository_json = json.loads(checkout_information_json)

What is this comment doing?

> Tools/Scripts/webkitpy/common/checkout/scm/stub_repository_unittest.py:56
> +        self.assertEqual(repository.native_revision(path=repository._filesystem.getcwd()), checkout_information_json['id'])

I think this should be host.filesystem.getcwd(), instead of using the private _filesystem variable

> Tools/Scripts/webkitpy/common/checkout/scm/stub_repository_unittest.py:71
> +        self.assertEquals(repository.find_checkout_root(path=repository._filesystem.getcwd()), host.filesystem.getcwd())

In order to truly test this, you need to change the cwd to some directory within the repository....which you'll probably need to create
Comment 17 Matt Lewis 2019-09-05 18:33:13 PDT
Created attachment 378146 [details]
Patch
Comment 18 Jonathan Bedard 2019-09-06 08:06:35 PDT
Comment on attachment 378146 [details]
Patch

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

Looks like all the logic is here, just a few nits.

> Tools/Scripts/webkitpy/common/checkout/scm/stub_repository.py:50
> +            path = filesystem.dirname(filesystem.abspath(path))

I think we should do path = filesystem.abspath(path) before the loop, so that the first iteration of the loop passes an absolute path to the callback too. If we do that we shouldn't need to call abspath in ever loop iteration. Existing unit tests should easily easily cover this if I'm wrong.

> Tools/Scripts/webkitpy/common/checkout/scm/stub_repository.py:58
> +        return self._find_parent_path_matching_callback_condition(path, lambda path: self._decode_json(path)['id'], filesystem=self._filesystem)

This should call self.native_revision(...) as to not duplicate code.

> Tools/Scripts/webkitpy/common/checkout/scm/stub_repository_unittest.py:31
> +             '/TestDirectory/checkout_information.json': '{ "branch": "trunk", "id": "2738499" }',

Just do a single indent, don't align with the bracket

> Tools/Scripts/webkitpy/common/checkout/scm/stub_repository_unittest.py:66
> +        self.assertEquals(repository.find_checkout_root(path=host.join('TestDirectory', 'TestDirectory2')), host.join(host.getcwd(), 'TestDirectory'))

Can we test the failure case too? (ie, we pass find_checkout_root '/' and get back None)
Comment 19 Jonathan Bedard 2019-09-06 08:09:27 PDT
Comment on attachment 378146 [details]
Patch

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

> Tools/Scripts/webkitpy/common/checkout/scm/stub_repository_unittest.py:33
> +             '/TestDirectory/TestDirectory2/test2.txt': 'test'

Missing , at the end of this line
Comment 20 Matt Lewis 2019-09-06 09:17:01 PDT
Created attachment 378197 [details]
Patch
Comment 21 Matt Lewis 2019-09-06 09:31:18 PDT
Created attachment 378199 [details]
Patch

I miissed a piece of the patch on the first upload.
Comment 22 Jonathan Bedard 2019-09-06 09:34:17 PDT
Comment on attachment 378199 [details]
Patch

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

> Tools/ChangeLog:33
> +

Make sure the first letter of each sentence is capitalized, don't indent 2nd line, always end sentances with a period (even if they aren't completeO)

> Tools/Scripts/webkitpy/common/checkout/scm/stub_repository.py:51
> +            path = filesystem.dirname(filesystem.abspath(path))

Probably don't need to re-call abspath every time, although it also isn't going to be harmful.

> Tools/Scripts/webkitpy/common/checkout/scm/stub_repository.py:59
> +        # return self._find_parent_path_matching_callback_condition(path, lambda path: self._decode_json(path)['id'], filesystem=self._filesystem)

Remove comment
Comment 23 Matt Lewis 2019-09-06 09:48:33 PDT
Created attachment 378202 [details]
Patch
Comment 24 WebKit Commit Bot 2019-09-06 11:10:55 PDT
Comment on attachment 378202 [details]
Patch

Clearing flags on attachment: 378202

Committed r249582: <https://trac.webkit.org/changeset/249582>
Comment 25 WebKit Commit Bot 2019-09-06 11:10:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 26 Matt Lewis 2019-09-06 17:42:40 PDT
Reverted r249582 for reason:

This caused results.html fail to be created on internal testers.

Committed r249602: <https://trac.webkit.org/changeset/249602>
Comment 27 Matt Lewis 2019-09-10 13:39:37 PDT
Created attachment 378481 [details]
Patch

I've tested the change a bit more and found that we were hitting an loop in the find_checkout_root function that was being triggered due to being passed a path that is 1 level higher than the checkout and never bailing when hitting the root directory.
Comment 28 Matt Lewis 2019-09-10 14:19:20 PDT
Created attachment 378492 [details]
Patch

Added tests for the find_parent_path_matching_callback_condition function.
Comment 29 Jonathan Bedard 2019-09-10 14:37:13 PDT
Comment on attachment 378492 [details]
Patch

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

> Tools/Scripts/webkitpy/common/checkout/scm/stub_repository.py:51
> +            if path == filesystem.dirname(path):

I think we should do this with a temp variable to avoid multiple calls to dirname. Something like this:

previous_path = ''
path = filesystem.abspath(path)

while path and path != previous_path:
    if filesystem.exists(filesystem.join(path, cls._stub_repository_json)):
        return callback(path)
    path = filesystem.dirname(path)
Comment 30 Matt Lewis 2019-09-10 15:22:28 PDT
(In reply to Jonathan Bedard from comment #29)
> Comment on attachment 378492 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=378492&action=review
> 
> > Tools/Scripts/webkitpy/common/checkout/scm/stub_repository.py:51
> > +            if path == filesystem.dirname(path):
> 
> I think we should do this with a temp variable to avoid multiple calls to
> dirname. Something like this:
> 
> previous_path = ''
> path = filesystem.abspath(path)
> 
> while path and path != previous_path:
>     if filesystem.exists(filesystem.join(path, cls._stub_repository_json)):
>         return callback(path)
>     path = filesystem.dirname(path)

This wont work, but I'm uploading a patch that should integrate this idea correctly.
Comment 31 Matt Lewis 2019-09-10 15:22:58 PDT
Created attachment 378500 [details]
Patch
Comment 32 Jonathan Bedard 2019-09-10 16:19:52 PDT
Comment on attachment 378500 [details]
Patch

Let's try this again...
Comment 33 Matt Lewis 2019-09-10 17:25:19 PDT
Comment on attachment 378500 [details]
Patch

Here goes round 2.
Comment 34 WebKit Commit Bot 2019-09-10 18:12:44 PDT
Comment on attachment 378500 [details]
Patch

Clearing flags on attachment: 378500

Committed r249749: <https://trac.webkit.org/changeset/249749>
Comment 35 WebKit Commit Bot 2019-09-10 18:12:46 PDT
All reviewed patches have been landed.  Closing bug.