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

fix(ci): Speed up CI, Add t.Parallel and Fix Test Parallelism Issue #2459

Merged
merged 5 commits into from
Feb 1, 2025

Conversation

rezbera
Copy link
Contributor

@rezbera rezbera commented Feb 1, 2025

CI got extremely slow because the -race flag and quick.Check don't like eachother. This fixes it by running the quick.Check tests without -race.

CI time goes from ~9 minutes to ~4

Contains :

Copy link

codecov bot commented Feb 1, 2025

Codecov Report

Attention: Patch coverage is 85.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 32.37%. Comparing base (a908148) to head (3407b15).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
consensus-types/types/payload.go 80.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #2459       +/-   ##
===========================================
+ Coverage   21.17%   32.37%   +11.19%     
===========================================
  Files           3      350      +347     
  Lines          85    15589    +15504     
  Branches       20       20               
===========================================
+ Hits           18     5047     +5029     
- Misses         66    10179    +10113     
- Partials        1      363      +362     
Files with missing lines Coverage Δ
consensus-types/types/signed_beacon_block.go 80.00% <100.00%> (ø)
payload/builder/payload.go 20.52% <100.00%> (ø)
consensus-types/types/payload.go 92.65% <80.00%> (ø)

... and 344 files with indirect coverage changes

@rezbera rezbera marked this pull request as ready for review February 1, 2025 04:57
@rezbera rezbera requested a review from a team as a code owner February 1, 2025 04:57
@rezbera rezbera enabled auto-merge (squash) February 1, 2025 04:57
Comment on lines +267 to +268
test-unit-cover: test-unit-norace ## run golang unit tests with coverage
@echo "Running unit tests with coverage and race checks..."
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unclear to me if there's a better way to separate tests. Its even unclear why we have to run tests with tags bls12381,test in the first place

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to include the test tag in go test for it to include and test the *_test.go files that import the statetransition package, see #2428

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do we need tags at all? build tags are quite undiscoverable - see. I'm also having trouble filtering tests.

The intention for test-unit-norace was that it only run tests built with norace - but it seems to be running all of them according to run logs.

Still faster than before though

ok  	github.com/berachain/beacon-kit/storage/beacondb	0.038s	coverage: 26.0% of statements
ok  	github.com/berachain/beacon-kit/storage/beacondb/index	0.008s	coverage: 0.0% of statements
ok  	github.com/berachain/beacon-kit/storage/block	0.005s	coverage: 82.8% of statements
ok  	github.com/berachain/beacon-kit/storage/filedb	0.026s	coverage: 77.1% of statements

@rezbera rezbera changed the title fix(ci): Speed up CI fix(ci): Speed up CI by Removing Race Flag for quick.Check Feb 1, 2025
Signed-off-by: Alberto Benegiamo <[email protected]>
Co-authored-by: Alberto Benegiamo <[email protected]>
@rezbera rezbera changed the title fix(ci): Speed up CI by Removing Race Flag for quick.Check fix(ci): Speed up CI, Add t.Parallel and Fix Test Parallelism Issue Feb 1, 2025
@rezbera rezbera disabled auto-merge February 1, 2025 21:08
@rezbera rezbera enabled auto-merge (squash) February 1, 2025 21:08
Copy link
Collaborator

@abi87 abi87 left a comment

Choose a reason for hiding this comment

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

This is in improvement wrt current state so I approve this.
We can iterate on top of this to solve the open points

@rezbera rezbera merged commit d4cf42b into main Feb 1, 2025
19 checks passed
@rezbera rezbera deleted the speed-up-ci-v1 branch February 1, 2025 21:12
shotes pushed a commit that referenced this pull request Feb 3, 2025
shotes pushed a commit that referenced this pull request Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants