You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
CI does not shuffle, so this is invisible there. It has surfaced repeatedly during review of #359 and #360 and is not caused by either — both were checked across ten seeds and every test they add passes.
Why it matters more than a flaky test
The package keeps its command-line state in package-level vars that cobra parses into: outputFmt, noColor, wide, outFile, selectFields, compact, fieldName, quiet, noHints, plus the auth ones. Any test that drives the root command leaves them set for whatever runs next.
That is not hypothetical. It produced a real false result during review:
go test -shuffle=4 failed a test in fix(output): print every command through the shared output formatter #359 with fieldName still "command" from an earlier runRoot, which sent printRows down the --field branch instead of rendering the table the test asserted on. Reproduced with no source change at all — the test was correct and the ordering was the bug.
Separately, TestBackupListResources_HonoursSelect leaked --select resource, and running it before the export test turned that test's buffer into [] instead of the expected YAML.
Both are fixed in #359 by a restoreOutputFlags(t) helper that saves and restores all nine output flag vars. The auth and profile vars have no equivalent, which is what the remaining failure is about.
Suggested direction
Two options, in increasing order of effort:
A shared t.Cleanup helper for the auth vars, matching restoreOutputFlags in output_flags_test.go, applied to every test that constructs a root command. Cheap, and it makes the package shuffle-clean.
Stop keeping flag state in package vars. They exist because generated commands reach them through CLIContext, so this is a wider change; option 1 is the pragmatic fix.
Either way, adding -shuffle=on to CI is what stops it coming back — but only after the existing failures are fixed, or CI becomes flaky.
Also present
The review that surfaced this reports two further order-dependent failures in the package at seeds 2 and 5, unrelated to the output flags. Worth running a sweep of seeds once the above lands rather than fixing one at a time.
What happens
go test ./internal/commands/ -shuffle=Nfails on seeds 2, 7 and 10 (and 1234, recorded earlier):CI does not shuffle, so this is invisible there. It has surfaced repeatedly during review of #359 and #360 and is not caused by either — both were checked across ten seeds and every test they add passes.
Why it matters more than a flaky test
The package keeps its command-line state in package-level vars that cobra parses into:
outputFmt,noColor,wide,outFile,selectFields,compact,fieldName,quiet,noHints, plus the auth ones. Any test that drives the root command leaves them set for whatever runs next.That is not hypothetical. It produced a real false result during review:
go test -shuffle=4failed a test in fix(output): print every command through the shared output formatter #359 withfieldNamestill"command"from an earlierrunRoot, which sentprintRowsdown the--fieldbranch instead of rendering the table the test asserted on. Reproduced with no source change at all — the test was correct and the ordering was the bug.TestBackupListResources_HonoursSelectleaked--select resource, and running it before the export test turned that test's buffer into[]instead of the expected YAML.Both are fixed in #359 by a
restoreOutputFlags(t)helper that saves and restores all nine output flag vars. The auth and profile vars have no equivalent, which is what the remaining failure is about.Suggested direction
Two options, in increasing order of effort:
t.Cleanuphelper for the auth vars, matchingrestoreOutputFlagsinoutput_flags_test.go, applied to every test that constructs a root command. Cheap, and it makes the package shuffle-clean.CLIContext, so this is a wider change; option 1 is the pragmatic fix.Either way, adding
-shuffle=onto CI is what stops it coming back — but only after the existing failures are fixed, or CI becomes flaky.Also present
The review that surfaced this reports two further order-dependent failures in the package at seeds 2 and 5, unrelated to the output flags. Worth running a sweep of seeds once the above lands rather than fixing one at a time.