[v2.3.2] Don't retry tests at the GHA level #4281
Merged
Conversation
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
Signed-off-by: Luke Shumaker <[email protected]>
Besides test retries being bad (they're there for the case that the test intermittently flakes, but mask the case that the code-under-test only intermittently works), this is important in order to remove the `nick-invision/[email protected]` GitHub action. It has a bug where if there is too much output it marks the test as passing even if it fails. That is very bad, and we cannot permit that type of failure mode in our CI. nick-fields/retry#76 Signed-off-by: Luke Shumaker <[email protected]>
fetcher.py's parse_watt() calls `watt_k8s.pop('annotations') or {}` to
pluck the annotations out of the watt snapshot. Because the `pop`
call does not include a default this causes it to throw a KeyError
when `annotations` doesn't exist. This isn't an issue in normal use,
as `pkg/snapshot/v1/types.go` doesn't say `,omitempty` on that field,
so it will always be present. However this does cause a problem for
many of the unit tests, which don't set that field. So, move the
default value to inside of the call to `pop`.
Signed-off-by: Lance Austin <[email protected]>
Signed-off-by: Luke Shumaker <[email protected]>
The test YAML parser doesn't need to be told about Namespaces; and indeed it chokes on them because they don't have a `.metadata.namespace`. Also, you can't multi-assign from a bool. Signed-off-by: Luke Shumaker <[email protected]>
Remove the Python unit test for testing annotation parsing because it is already being tested in `pkg/snapshot/v1/annotations_test.go`. However, the Go version is missing tests for the following use cases: - "getambassador.io/config" annotation is empty - "getambassador.io/config" is empty missing So add those 2 cases to the Go test. Signed-off-by: Lance Austin <[email protected]> Signed-off-by: Luke Shumaker <[email protected]>
Fixes: 417a831 Signed-off-by: Luke Shumaker <[email protected]>
Make it clear that "XXX" in this case means that something is wrong. Signed-off-by: Luke Shumaker <[email protected]>
LukeShu
force-pushed
the
lukeshu/no-gha-retry
branch
from
3ff74b6 to
a1810d6
Compare
LanceEa
approved these changes
Jun 15, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Stop using the
nick-invision/[email protected]GitHub action to retry tests. Besides test retries being bad (they're there for the case that the test intermittently flakes, but mask the case that the code-under-test only intermittently works), this action in particular is problematic: It has a bug (nick-fields/retry#76) where if there is too much output it marks the test as passing even if it fails. That is very bad, and we cannot permit that type of failure mode in our CI.This reveals a number of tests that were failing but we didn't know it. Spend several commits fixing those.
Testing
That's pretty much all this is.
Checklist
I made sure to update
CHANGELOG.md. - no applicable changesRemember, the CHANGELOG needs to mention:
This is unlikely to impact how Ambassador performs at scale.
Remember, things that might have an impact at scale include:
My change is adequately tested.
Remember when considering testing:
I updated
DEVELOPING.mdwith any any special dev tricks I had to use to work on this code efficiently. - no tricksThe changes in this PR have been reviewed for security concerns and adherence to security best practices.