Bug 207684 – Stub repositories fail to upload some results due to missing head svn revision

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Matt Lewis]

Currently we fail to upload some results when running tests from a stub repository because we expect to be able to get the head for a given svn checkout. We need to make it possible to get this information from the checkout_information.json

I'm proposing we just add an additional entry to the json for 'head': '<revision>' to help with getting the required information.

[Radar WebKit Bug Importer]

<rdar://problem/59414903>

[Matt Lewis]

Created attachment 390649 [details]
Patch

I believe this should fix the issues while maintaining functionality.

[Jonathan Bedard]

Comment on attachment 390649 [details]
Patch

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

> Tools/Scripts/webkitpy/common/checkout/scm/git.py:295
> +        return self.svn_revision(path)

I'm a bit confused. What does webkitpy think the difference between a head revision and the svn revision is?

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

I think that the if you don't have a HEAD in the json, you should default to the native_revision(...)

[Matt Lewis]

We discovered that there is a better way to fix this issue than the proposed one due to the outdated code. As such a slightly bigger change is needed.

[Matt Lewis]

Created attachment 391073 [details]
Patch

Though this doesn't seem like a large change, this has a bigger impact. In multiple spots we are determining our root directory off the actual host running the test suites. This basically made our unit test for certain areas using the mock filesystem and SCMs worthless.

[Jonathan Bedard]

Comment on attachment 391073 [details]
Patch

Matt and I talked about this change this morning. This looks correct, although we should be careful landing because fully testing upload changes is difficult, I know we've been bitten by that a few times in the last 2-3 months.

[Matt Lewis]

Committed r256851: <https://trac.webkit.org/changeset/256851>

[Ryan Haddad]

Reverted r256851 for reason:

Broke internal bots

Committed r256894: <https://trac.webkit.org/changeset/256894>

[Ryan Haddad]

(In reply to Ryan Haddad from comment #8)
> Reverted r256851 for reason:
> 
> Broke internal bots
> 
> Committed r256894: <https://trac.webkit.org/changeset/256894>
Details in radar.

[Matt Lewis]

Created attachment 391302 [details]
Patch

Still needs testing.

[Matt Lewis]

Comment on attachment 391302 [details]
Patch

Testing show this should work.

[Jonathan Bedard]

Comment on attachment 391302 [details]
Patch

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

I don't think we have correctness issues anymore.

I think we need to think about script performance for a moment, though. initialie_scm() is a relatively expensive call and should be avoided unless we really need it.

> Tools/Scripts/webkitpy/api_tests/run_api_tests.py:44
> +    host.initialize_scm()

Do we actually need this?

It's a relatively expensive call, this patch means that we're running it even when we pass --help.

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:67
> +        host.initialize_scm()

What happens if we call this twice in a test run? Does the function actually run twice?

I ask because initializing the sum is actually a relatively expensive task, it feels like we should reserve it for the commits_for_upload call.

> Tools/Scripts/webkitpy/test/main.py:61
> +    _webkit_root = _host.scm().checkout_root

I know I r+ed something like this earlier, but I wonder if we're actually making the right call here.

We don't need the SCM to run the tests, we only need it for reporting. If we want to make changes to this file, I think we should do it in another patch. This change (in my opinion) would make things worse in test-webkitpy.

[Matt Lewis]

Created attachment 391341 [details]
Patch

Still needs testing.

[Matt Lewis]

Created attachment 391348 [details]
Patch

Good point about the initialize stuff. We now only call it when uploading. Unfortunately we still have to detect the SCM per repo to get the right type, but this allows it to work in all types of SCMs that we might be using, or lack there of.I think we got it this time.

[Jonathan Bedard]

Comment on attachment 391348 [details]
Patch

I talked to Matt about this offline, he's going to refrain landing it until tomorrow morning.

[Matt Lewis]

Committed r257144: <https://trac.webkit.org/changeset/257144>