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

Refactor gotestsum "test one package at a time" procedure #2611

Merged
merged 5 commits into from
Sep 30, 2024

Conversation

diegoximenes
Copy link
Contributor

@diegoximenes diegoximenes commented Aug 26, 2024

Resolves NIT-2693

Moves gotestsum call to a bash script to avoid code duplication.

@cla-bot cla-bot bot added the s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA. label Aug 26, 2024
@diegoximenes diegoximenes force-pushed the gotestsum_improvement branch 6 times, most recently from 25ddacc to 0689ae8 Compare August 26, 2024 17:40
@diegoximenes diegoximenes marked this pull request as ready for review August 26, 2024 17:52
Copy link

codecov bot commented Aug 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 23.41%. Comparing base (16f41be) to head (0689ae8).
Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2611      +/-   ##
==========================================
+ Coverage   23.06%   23.41%   +0.35%     
==========================================
  Files         260      260              
  Lines       37354    37482     +128     
==========================================
+ Hits         8615     8778     +163     
+ Misses      27290    27255      -35     
  Partials     1449     1449              

eljobe
eljobe previously approved these changes Aug 29, 2024
Copy link
Member

@eljobe eljobe left a comment

Choose a reason for hiding this comment

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

LGTM

.github/workflows/ci.yml Outdated Show resolved Hide resolved
eljobe
eljobe previously approved these changes Aug 30, 2024
Copy link
Member

@eljobe eljobe left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@tsahee tsahee left a comment

Choose a reason for hiding this comment

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

small comment. otherwise seems good

cmd="$cmd -coverprofile=coverage.txt -covermode=atomic -coverpkg=./...,./go-ethereum/..."
fi

if [ "$write_full_log" == true ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this matches previous behaviour, but there is no good reason to evern -not- do that. Meaning, the stdbuf trick should be set on all invocations.
Possibly, someone may want to run the script on his own computer and only in this case they won't want to use --write-full-log

Copy link
Contributor Author

@diegoximenes diegoximenes Sep 23, 2024

Choose a reason for hiding this comment

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

All invocations write to full.log now

Copy link
Contributor

@tsahee tsahee left a comment

Choose a reason for hiding this comment

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

LGTM

@tsahee tsahee merged commit f5fb27b into master Sep 30, 2024
15 checks passed
@tsahee tsahee deleted the gotestsum_improvement branch September 30, 2024 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design-approved s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants