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

test: Move instrumentation to AfterFailed instead of AfterAll #16845

Conversation

christarazi
Copy link
Member

When inspecting a sysdump of this failing test, the expected artifacts
containing the instrumentation (debug-events) are not present. This is
because AfterAll is not called when the test fails. The logic should be
in AfterFailed.

Fixes: e63aa2b ("test: Instrument LB IP via BGP test with debug-events")
Fixes: #16445

Signed-off-by: Chris Tarazi chris@isovalent.com

When inspecting a sysdump of this failing test, the expected artifacts
containing the instrumentation (debug-events) are not present. This is
because AfterAll is not called when the test fails. The logic should be
in AfterFailed.

Fixes: e63aa2b ("test: Instrument LB IP via BGP test with debug-events")
Fixes: cilium#16445

Signed-off-by: Chris Tarazi <chris@isovalent.com>
@christarazi christarazi added area/CI Continuous Integration testing issue or flake needs-backport/1.10 release-note/ci This PR makes changes to the CI. labels Jul 9, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jul 9, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.10.3 Jul 9, 2021
@christarazi
Copy link
Member Author

test-1.16-netnext

@christarazi christarazi marked this pull request as ready for review July 9, 2021 17:46
@christarazi christarazi requested a review from a team as a code owner July 9, 2021 17:46
@christarazi christarazi requested review from a team and nbusseneau July 9, 2021 17:46
@christarazi christarazi requested a review from jibi July 9, 2021 17:46
Copy link
Member

@nbusseneau nbusseneau left a comment

Choose a reason for hiding this comment

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

Good catch.

@christarazi
Copy link
Member Author

Approving reviews are in and affected CI job has passed, marking ready to merge.

@christarazi christarazi added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 12, 2021
@kkourt kkourt merged commit 9744ff0 into cilium:master Jul 13, 2021
@christarazi christarazi deleted the pr/christarazi/fix-instrument-debug-events-lb-ip branch July 13, 2021 17:21
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport done to v1.10 in 1.10.3 Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/CI Continuous Integration testing issue or flake ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/ci This PR makes changes to the CI.
Projects
No open projects
1.10.3
Backport done to v1.10
Development

Successfully merging this pull request may close these issues.

None yet

5 participants