-
Notifications
You must be signed in to change notification settings - Fork 158
Optimize build_test.sh to not rereun tests on flake failure #1065
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
Open
JonathanOppenheimer
wants to merge
44
commits into
master
Choose a base branch
from
rewrite-test-retry-logic
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 29 commits
Commits
Show all changes
44 commits
Select commit
Hold shift + click to select a range
7697a6c
rewrite test retry logic
JonathanOppenheimer 0f1e686
only rerun failed + non-run tests
JonathanOppenheimer d5f0d87
rewrite get all tests for package level
JonathanOppenheimer 43f77e0
try retry at individual package level
JonathanOppenheimer 4ec0caf
add debug
JonathanOppenheimer eeb1a25
rewrite get_all_tests
JonathanOppenheimer df4d5f2
enhancements
JonathanOppenheimer ffbe3e4
fix RAN TESTS
JonathanOppenheimer 5db5447
hmmm
JonathanOppenheimer 93d7334
try this
JonathanOppenheimer b7f68e6
fix env vars
JonathanOppenheimer b0fb02f
change coreth path
JonathanOppenheimer aac7eaf
more efficient get test list
JonathanOppenheimer 6b3e797
add echo statements
JonathanOppenheimer 998bbdf
ADSFADSF
JonathanOppenheimer 388c630
typo
JonathanOppenheimer 024ab2a
Merge branch 'master' into rewrite-test-retry-logic
JonathanOppenheimer 5d18f5f
restore test coverage
JonathanOppenheimer 3cd402a
simplify
JonathanOppenheimer 3f28633
got
JonathanOppenheimer 5226e5a
no eit
JonathanOppenheimer 3496271
add -v
JonathanOppenheimer e476499
use json
JonathanOppenheimer 7d38556
gotestsum
JonathanOppenheimer 528c57c
RESET
JonathanOppenheimer fa233b2
try rerun at the package level
JonathanOppenheimer 9a6f4fa
lint errors
JonathanOppenheimer a6893d0
seperate paths
JonathanOppenheimer 50d090a
Merge branch 'master' into rewrite-test-retry-logic
JonathanOppenheimer 3c46681
save package for test through mapping:
JonathanOppenheimer 70a34c8
remove non-flaky test
JonathanOppenheimer 58ca623
Merge branch 'master' into rewrite-test-retry-logic
JonathanOppenheimer 48d8f60
Merge branch 'master' into rewrite-test-retry-logic
JonathanOppenheimer 54ab582
See bash version
JonathanOppenheimer d78e10b
try other bash
JonathanOppenheimer ccd8f70
comply with old bash version
JonathanOppenheimer 1dc934c
Merge branch 'master' into rewrite-test-retry-logic
JonathanOppenheimer dd94616
Merge branch 'master' into rewrite-test-retry-logic
JonathanOppenheimer 5c8fdca
Merge branch 'master' into rewrite-test-retry-logic
JonathanOppenheimer b68485a
Merge branch 'master' into rewrite-test-retry-logic
JonathanOppenheimer 2a6fdb9
Merge branch 'master' into rewrite-test-retry-logic
JonathanOppenheimer db3a694
Merge branch 'master' into rewrite-test-retry-logic
JonathanOppenheimer 70512e3
Merge branch 'master' into rewrite-test-retry-logic
JonathanOppenheimer 785468b
Merge branch 'master' into rewrite-test-retry-logic
JonathanOppenheimer File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Not sure if it's possible to write this line so it both
A. works
B. passes lint
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.
How about separating creation of the array from its use?
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.
If only! Writing this script was actually quite difficult because while bash is a decent programming language (imo).
mapfile
was added in Bash 4.0 and all supported macOS releases (including the ones behind GitHub Actions’ macos‑latest runner) still ship with Bash 3.2.x—currently 3.2.57—because Apple never adopted the GPL v3‑licensed Bash 4+ and instead moved its interactive shell to zsh while leaving /bin/bash frozen at 3.2.57(see https://jmmv.dev/2019/11/macos-bash-baggage.html)
Thus all of our testing scripts need to be compatible with bash circa 2014, and I was hindered from using a lot of newer bash features that would have made this easier to write.
If you endorse it, I can refactor the whole testing script to use Python (or GoLang) instead of bash but it would be a much more significant change.
Uh oh!
There was an error while loading. Please reload this page.
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.
There is no obligation to be compatible with macos's ancient bash version. In avalanchego, we explicitly require a modern bash. One way to ensure the use of modern bash could be to run the command with
scripts/dev_shell.sh
, since the nix shell guarantees a compatible bash version.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.
Interesting! That's definitely something that is worth bringing over to the evm repositories in my opinion.