Repository navigation
Defer AfterAll until repeated spec completes - #1681
Merged
onsi merged 1 commit intoAug 10, 2026
Merged
Conversation
Owner
|
code lgtm thanks! Will pull this in once CI goes green. |
Contributor
Author
|
Thanks for the review! I'm checking the oldstable integration failure now and will update once I understand what's happening. |
Owner
|
cool, thanks. if it's an unrelated flake don't worry about it. |
ilovelinux
approved these changes
Aug 10, 2026
There was a problem hiding this comment.
I double-checked the implemented tests empirically:
$ git remote add -f Boulea7 https://github.com/Boulea7/ginkgo
$ git switch fix/ordered-afterall-must-pass-repeatedly
$ git restore --source master internal/group.go # Reverse the bugfix
$ ginkgo -r -p # seed: 1786398309
[...]
Summarizing 4 Failures:
[FAIL] Running tests in parallel [It] distributes tests across the parallel procs and runs them
/home/user/projects/github/onsi/ginkgo/internal/internal_integration/parallel_test.go:159
[FAIL] Ordered Containers [It] when the final spec must pass repeatedly, it runs BeforeAll and AfterAll just once
/home/user/projects/github/onsi/ginkgo/internal/internal_integration/ordered_test.go:61
[FAIL] Ordered Containers [It] when a non-final spec must pass repeatedly, it runs BeforeAll and AfterAll just once
/home/user/projects/github/onsi/ginkgo/internal/internal_integration/ordered_test.go:61
[FAIL] Ordered Containers [It] when the final spec fails while it must pass repeatedly, it runs AfterAll after the failure
/home/user/projects/github/onsi/ginkgo/internal/internal_integration/ordered_test.go:61
Ran 370 of 370 Specs in 2.579 seconds
FAIL! -- 366 Passed | 4 Failed | 0 Pending | 0 Skipped
[...]They fail as expected! 😄
LGTM!
Owner
|
just shipped 2.32.1 with this in it. thanks! |
Contributor
Author
|
That's great to hear—thank you for merging it and shipping the fix in v2.32.1! |
1 task
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
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.
Fixes #1680.
AfterAllis a run-once teardown for anOrderedcontainer. When the final spec usedMustPassRepeatedly, each intermediate successful attempt was treated as the last opportunity to run teardown. This causedAfterAllto run before the next attempt and potentially more than once.This change bases the run-once after-node decision on whether the active repeat or retry policy will schedule another attempt. A passing
MustPassRepeatedlyattempt defers teardown while more required passes remain. For the final spec, teardown runs after all required passes, or after an early failure when repetition stops.FlakeAttemptskeeps its existing inverse behavior: failed attempts defer teardown while they can be retried, and a passing attempt completes it.The integration coverage records exact attempt events and node order for:
BeforeAll/AfterAlland per-attemptBeforeEach/AfterEachSerialordered containersValidation performed locally with Go 1.26.5:
internal/...packages other than the fullinternal_integrationpackagego vet ./...The full
internal_integrationpackage reports 369 of 370 specs passing. The remaining unchanged timeout test compares aCodeLocationwhoseFullStackTraceis populated while the assertion expects it to be empty. The same focused failure reproduces at the unmodified base commit09bbf5fwith Go 1.26.5.