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.
<rdar://problem/59414903>
Created attachment 390649 [details] Patch I believe this should fix the issues while maintaining functionality.
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(...)
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.
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.
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.
Committed r256851: <https://trac.webkit.org/changeset/256851>
Reverted r256851 for reason: Broke internal bots Committed r256894: <https://trac.webkit.org/changeset/256894>
(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.
Created attachment 391302 [details] Patch Still needs testing.
Comment on attachment 391302 [details] Patch Testing show this should work.
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.
Created attachment 391341 [details] Patch Still needs testing.
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.
Comment on attachment 391348 [details] Patch I talked to Matt about this offline, he's going to refrain landing it until tomorrow morning.
Committed r257144: <https://trac.webkit.org/changeset/257144>