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

tests: disable TestInitialSync on CI #5053

Merged

Conversation

algorandskiy
Copy link
Contributor

Summary

TestInitialSync creates 10 full nodes that is too much for CI builder.

Test Plan

This is a test

Eric-Warehime
Eric-Warehime previously approved these changes Jan 24, 2023
@@ -245,6 +246,10 @@ func TestInitialSync(t *testing.T) {
t.Skip("Test takes ~25 seconds.")
}

if strings.ToUpper(os.Getenv("CIRCLECI")) == "TRUE" {
t.Skip("Test is too heavy for amd64 builder running in parallel with other packages")
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 really running in parallel with other packages though? I thought we don't allow that

Copy link
Contributor Author

@algorandskiy algorandskiy Jan 25, 2023

Choose a reason for hiding this comment

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

try make test and observe LA 100+ with all cores busy and 8+ test processes

Copy link
Contributor

Choose a reason for hiding this comment

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

Is far as I know, tests do not run with others when t.Parallel() is not called.
That the the whole reason why we have multiple runs on CircleCi.

If we are running things in parallel, something has changed!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

t.Parallel() takes effect only within a single package (test binary)

Copy link
Contributor

Choose a reason for hiding this comment

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

we disable package-level parallelism in CI by passing -p 1 in the .circleci/confiig.yml file though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, I expected it calling make test. Then it is very weird it fails time to time

Copy link
Contributor

@cce cce left a comment

Choose a reason for hiding this comment

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

Is it possible to run it with 5 instead of 10 full nodes instead?

or — nightly tests are running on medium (2 cores) could change the resource class used by test_nightly to run on large (4 cores) instead?

@codecov
Copy link

codecov bot commented Jan 24, 2023

Codecov Report

Merging #5053 (8e3b681) into master (92bb3ce) will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5053      +/-   ##
==========================================
- Coverage   53.57%   53.56%   -0.02%     
==========================================
  Files         430      430              
  Lines       54091    54091              
==========================================
- Hits        28979    28973       -6     
- Misses      22868    22872       +4     
- Partials     2244     2246       +2     
Impacted Files Coverage Δ
ledger/tracker.go 74.26% <0.00%> (-0.85%) ⬇️
ledger/catchpointtracker.go 57.70% <0.00%> (-0.80%) ⬇️
catchup/service.go 68.59% <0.00%> (-0.73%) ⬇️
ledger/acctupdates.go 68.99% <0.00%> (-0.25%) ⬇️
network/wsNetwork.go 64.92% <0.00%> (+0.17%) ⬆️
agreement/proposalManager.go 98.03% <0.00%> (+1.96%) ⬆️
agreement/cryptoVerifier.go 69.71% <0.00%> (+2.11%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

node/node_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@algonautshant algonautshant left a comment

Choose a reason for hiding this comment

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

Thanks for doing this.

A couple of more tests will have the same faith:
TestSyncingFullNode
TestSimpleUpgrade

@algorandskiy algorandskiy force-pushed the pavel/disable-test-initial-sync branch from 305eddf to 8e3b681 Compare January 25, 2023 01:16
@onetechnical onetechnical merged commit 40b8013 into algorand:master Jan 25, 2023
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.

5 participants