Bug 200787 – run-webkit-test: Allow results to be uploaded without scm checkout

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Matt Lewis]

run-webkit-test: Allow results to be uploaded without scm checkout

[Radar WebKit Bug Importer]

<rdar://problem/54364258>

[Matt Lewis]

Created attachment 376435 [details]
Patch

[Alexey Proskuryakov]

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?

[Matt Lewis]

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

[Jonathan Bedard]

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.

[Matt Lewis]

Created attachment 376821 [details]
Patch

[Jonathan Bedard]

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

[Matt Lewis]

Created attachment 376956 [details]
Patch

Work towards fixing a json and scm detection error

[Matt Lewis]

Created attachment 377141 [details]
Patch

Work toward consistently running

[Matt Lewis]

Created attachment 377284 [details]
Patch

[Matt Lewis]

Tested the changes with the stub repository and regular checkouts and worked locally.

[Jonathan Bedard]

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?

[Matt Lewis]

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

[Matt Lewis]

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

[Matt Lewis]

Created attachment 377593 [details]
Patch

Added unittesting for stub repository.

[Jonathan Bedard]

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

[Matt Lewis]

Created attachment 378146 [details]
Patch

[Jonathan Bedard]

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)

[Jonathan Bedard]

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

[Matt Lewis]

Created attachment 378197 [details]
Patch

[Matt Lewis]

Created attachment 378199 [details]
Patch

I miissed a piece of the patch on the first upload.

[Jonathan Bedard]

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

[Matt Lewis]

Created attachment 378202 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 378202 [details]
Patch

Clearing flags on attachment: 378202

Committed r249582: <https://trac.webkit.org/changeset/249582>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Matt Lewis]

Reverted r249582 for reason:

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

Committed r249602: <https://trac.webkit.org/changeset/249602>

[Matt Lewis]

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.

[Matt Lewis]

Created attachment 378492 [details]
Patch

Added tests for the find_parent_path_matching_callback_condition function.

[Jonathan Bedard]

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)

[Matt Lewis]

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

[Matt Lewis]

Created attachment 378500 [details]
Patch

[Jonathan Bedard]

Comment on attachment 378500 [details]
Patch

Let's try this again...

[Matt Lewis]

Comment on attachment 378500 [details]
Patch

Here goes round 2.

[WebKit Commit Bot]

Comment on attachment 378500 [details]
Patch

Clearing flags on attachment: 378500

Committed r249749: <https://trac.webkit.org/changeset/249749>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.