-
Notifications
You must be signed in to change notification settings - Fork 450
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
Conversation
25ddacc
to
0689ae8
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
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 |
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.
LGTM
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.
LGTM
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.
small comment. otherwise seems good
.github/workflows/gotestsum.sh
Outdated
cmd="$cmd -coverprofile=coverage.txt -covermode=atomic -coverpkg=./...,./go-ethereum/..." | ||
fi | ||
|
||
if [ "$write_full_log" == true ]; then |
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.
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
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.
All invocations write to full.log now
c2f9d9f
to
73da786
Compare
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.
LGTM
Resolves NIT-2693
Moves gotestsum call to a bash script to avoid code duplication.