-
Notifications
You must be signed in to change notification settings - Fork 399
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
chore: retry failed Circle CI tests #2814
Merged
Merged
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
1a8cdf2
chore: retry failed Circle CI tests
nolanlawson 18eaac0
chore: fix
nolanlawson 599d306
chore: fix
nolanlawson 04039e2
chore: fix
nolanlawson 3bb034e
chore: fix
nolanlawson fc033a5
chore: fix
nolanlawson f996626
chore: fix
nolanlawson f0763bb
chore: fix
nolanlawson 611fc34
chore: deliberately fail a test to see what happens
nolanlawson a170b83
chore: improve retry script
nolanlawson 993210f
fix: whitespace
nolanlawson 7ea1add
Revert "chore: deliberately fail a test to see what happens"
nolanlawson 301b0ef
chore: rename to retry_karma
nolanlawson 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 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
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.
Nitpick: It feels strange that the generic
retry_until_success
command acceptscoverage
,compat
,disable_synthetic
, andforce_native_shadow_mode
as parameters since those parameters are only applicable to the Karma tests.IMO it would be preferable to keep them inside the Kama command. It appears the CircleCi config syntax doesn't accept passing arbitrary objects. Many passing them as via a generic
environment
parameter would do the trick.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 considered using
environment
, but it's tricky because right now we are relying on the presence or absence of environment variables. E.g. I'm not sure if passingCOVERAGE=0
will actually disable coverage – I suspect it would be converted to a string"0"
in JavaScript, which is truthy, so coverage would be enabled. We would have to audit each of our environment variables to make sure they handle"0"
,"false"
, etc.I agree the name
retry_until_success
is not great. Mayberetry_karma_until_success
? Or justretry_karma
?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.
retry_karma
sounds good.You can set those environment variables to a nullish value like an empty string, which would coerce to a
false
boolean.While looking into this, I learned that bash allows you to define environment variables without any value. The following code would also work.
Again it's a nitpick.
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.
Renamed to
retry_karma
. I think I'd like to keep the existing parameter system, because it's a little unclear to me from reading the Circle CI docs how to pass an environment variable to a command. If there's a good way to refactor this, let's do it in a follow-up PR. 🙂