New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SubmoduleTests: Setup repos once for all tests #8334
SubmoduleTests: Setup repos once for all tests #8334
Conversation
Setup attributes for Parallelizable to disallow the individual tests in parallel
7de8f18
to
0e595c9
Compare
Codecov Report
@@ Coverage Diff @@
## master #8334 +/- ##
==========================================
- Coverage 52.77% 52.70% -0.07%
==========================================
Files 866 866
Lines 62691 62615 -76
Branches 11300 11289 -11
==========================================
- Hits 33083 33000 -83
- Misses 26965 26972 +7
Partials 2643 2643
Flags with carried forward coverage won't be shown. Click here to find out more. |
We should keep this in mind for all tests. CC: @sharwell |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick formal comparison with previous.
return; | ||
} | ||
|
||
_isInit = true; | ||
_repo1 = new GitModuleTestHelper("repo1"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests shouldn't concern themselves with resetting the state after execution. I was thinking of a change along the lines of ReferenceRepoistory
:
if (_repo1 is null)
{
_repo1 = new GitModuleTestHelper("repo1");
}
else
{
_repo1.Reset();
}
Do you think we can re-write these tests to utilise ReferenceRepository
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The repos would need not just to reset, but also undo commits. That would require that the initial commit have to be remembered, which will make a more complex setup. Also, a reset will increase the test time slightly.
The test cases cleaning up is much easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A clean is needed too, in addition to reset and checkout.
(cherry picked from commit 01bbd5d)
However on my work machine tests fail to stage and I need 649552a. |
The tests requiring a second basically runs UpdateSubmoduleStructureAndWaitForResultAsync() for top module (a series of commands parsing .gitmodules creating GitModules recursively) then a couple of git-status from top module and one or two other commands. Stepping in one test case, one time: Submodule_status_changes_for_top_module_with_first_nested_module_commit_second_nested_module_change Git status is cached, the time cannot be taken out of context However, the check that a repo is clean before and after adds 200 ms for this test. Slightly less for some, more for others. Unless there are requests to optimize, I plan to merge in a day or so. |
Thank you. This is an improvement, let's merge it.
|
Fixes #8279
Proposed changes
Setup the Git repos used in the tests only once, to speed up tests.
For me, test time goes down from 1,6 mon to 11s, about twice the time for one test. (Longer time for other users.)
More comments in the code
I consider this to be a major hack.
Test methodology
Tests only
Test environment(s)
✒️ I contribute this code under The Developer Certificate of Origin.