Skip to content
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

[#1349] Clone test repositories only once when testing #1478

Merged
merged 6 commits into from
Jun 3, 2021

Conversation

dcshzj
Copy link
Member

@dcshzj dcshzj commented Mar 14, 2021

Fixes #1349.

Tests and system tests are treated separately, as they represent different commands within Gradle.

The test repositories are cloned for every test that performs
cloning, which results in tests taking a long time to complete as
a significant amount of time is spent cloning the same repository
for every test.

Let's prevent the deletion of the cloned test repositories until
we are sure that all tests have completed, thus allowing the test
repositories to be cloned only once during each test run.

Additional information

The following table represents the comparison in build times before and after this change is implemented:

Test type Build time before change Build time after change Impact
Normal (test) 94s 52s Decrease by 44.68%
System test (systemTest) 149s 127s Decrease by 14.77%

The test repositories are cloned for every test that performs
cloning, which results in tests taking a long time to complete as
a significant amount of time is spent cloning the same repository
for every test.

Let's prevent the deletion of the cloned test repositories until
we are sure that all tests have completed, thus allowing the test
repositories to be cloned only once during each test run.
@dcshzj dcshzj requested a review from a team March 14, 2021 09:43

if (!SystemUtil.isTestEnvironment()) {
FileUtil.deleteDirectory(outputFolderPath.toString());
Files.createDirectories(rootPath);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible that you can add a test to cover this branch here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, as this branch can only run if it is a non-test environment. The test environment is defined in an environment variable and it won't be easy to modify it just for one test.

Copy link
Contributor

@HCY123902 HCY123902 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dcshzj dcshzj requested a review from a team March 23, 2021 06:15
@0blivious 0blivious requested review from Tejas2805 and removed request for a team March 23, 2021 10:29
Copy link
Contributor

@Tejas2805 Tejas2805 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One doubt.

Path repoPath = Paths.get(FileUtil.REPOS_ADDRESS, config.getRepoFolderName(), config.getRepoName());

if (SystemUtil.isTestEnvironment() && Files.exists(repoPath)) {
return true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this mean that the repo is already cloned? Then why are we returning true for wait?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the repo is already cloned at this stage. The true is to indicate that the function is successful, otherwise it will return false if an exception has occurred.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Tejas2805 Ping

@dcshzj dcshzj requested a review from Tejas2805 April 1, 2021 02:05
@fzdy1914
Copy link
Member

fzdy1914 commented Apr 5, 2021

@Tejas2805 You input is needed

@fzdy1914
Copy link
Member

@Tejas2805 Ping again

Copy link
Contributor

@Tejas2805 Tejas2805 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Sorry for the late reply.

@dcshzj dcshzj requested a review from a team May 21, 2021 02:15
@fzdy1914
Copy link
Member

Will try to merge it asap.

@dcshzj
Copy link
Member Author

dcshzj commented Jun 2, 2021

@fzdy1914 Will this be merged soon?

@fzdy1914
Copy link
Member

fzdy1914 commented Jun 3, 2021

Can you help check why the deployment fail? @dcshzj

@dcshzj
Copy link
Member Author

dcshzj commented Jun 3, 2021

Can you help check why the deployment fail? @dcshzj

@fzdy1914 It is likely some weird bug that occurred at random, it should be working now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test repositories should be cloned only once when testing
4 participants