test: rename the tag to post-mingo and put the script-history cases behind it - #2220
Merged
Merged
Conversation
…ehind it Two changes, one idea. `notification` -> `post-mingo` on NotificationsTest. The phase it selects is not about notifications, it is about running after the assistant phase, and a second class now needs the same placement. ScriptExecutionHistoryTest gets the tag. `Read a script's execution history and facets` has aborted on every nightly (09-15, 09-16, 09-17) with its own assumption spelling out why: "No script in this tenant has been dispatched yet, so there is no execution history to read." A pipeline run registers a fresh tenant, the only scripts ever dispatched on it are the ones the mingo phase runs, and this class sits in the catch-all `functional` phase, which is earlier. Exactly the shape of the notification aborts fixed in #2211. Tagged at class level rather than on the one aborting method. Its sibling, `Read a schedule's executions, runs and facets`, passes today and does not need to move, but splitting a class across two phases is what made `ExtApi: Archive customer` fail on its second run against static fixture state (#2215). Keeping the class inside a single phase is worth more than leaving one passing case where it is. Needs the matching saas-shared rename to land with the release that carries this; on its own the `notification` phase stops matching and both classes fall back to `functional`, so the two must ship together. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
🦩 Flamingo Code ReviewNo findings on the current head. Mode: advisory Need another pass? Commits pushed after this review are not reviewed automatically.
Prefer typing? Comment React 👍/👎 on inline comments to teach the reviewer. Started 2026-09-17 05:48 UTC · updated 2026-09-17 05:49 UTC · workflow run |
giokur
enabled auto-merge (squash)
September 17, 2026 06:29
mikhail-nosan
approved these changes
Sep 17, 2026
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
2 participants
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.
Two changes, one idea.
Rename:
notification→post-mingoThe phase added in #2211 is not about notifications. It is about running after the assistant phase,
and a second class now needs the same placement, so the tag should say what it means.
ScriptExecutionHistoryTestjoins itRead a script's execution history and facetshas aborted on every nightly — 09-15, 09-16 and09-17. Its own assumption says why:
Same shape as the notification aborts. A pipeline run registers a fresh tenant, the only scripts ever
dispatched on it are the ones the
mingophase runs, and this class sits in the catch-allfunctionalphase, which is earlier.It never turned a run red, because an abort is not a failure. That is precisely why it sat there for
three nightlies while the notification one next to it got fixed.
Why the tag is on the class, not the method
Only
testScriptExecutionsaborts. Its siblingRead a schedule's executions, runs and facetspassestoday and does not need to move.
Tagging just the one method would split the class across two phases, and that is exactly what made
ExtApi: Archive customerfail on its second run against stale static fixture state (#2215). Keepinga class inside a single phase is worth more than leaving one passing case where it is. The sibling
only gets richer data after
mingo, so nothing is lost.Sequencing — these must ship together
On its own this PR breaks the placement: the
notificationphase in saas-shared stops matchinganything, and both classes fall back into
functional. The matching saas-shared rename is preparedand will land with the pin bump to whatever release carries this commit, in one PR, the way #2141 did.
So: merge and release this, then the shared side goes in atomically. Nothing regresses in between
because the shared side is not merged until the release exists.
Verified
Module compiles. No
@Tag("notification")remains anywhere in the test library.🤖 Generated with Claude Code