Fix multiple calls to t.Parallel in cleanup test #791
Conversation
t.Parallel() was being called multiple times on the same parent test t. The testcase.Init() method calls t.Parallel(), and it was being called in a loop with the parent test, causing a panic to occur. This moves the testcase initialization inside the t.Run callback so that Init() (and thus t.Parallel()) is called on each subtest's *testing.T instead of the parent test. This also removes the unused sync.WaitGroup variable and import.
|
This SEEMED to fix it up; it tidied up 100 pipelines when it ran (https://buildkite.com/buildkite-kubernetes-stack/kubernetes-agent-stack/builds/3418/steps/canvas?sid=019b497f-5377-4bf9-a82f-e7535c5bee37) But my Golang is weak, and perhaps I'm misreading the code (should it have cleaned up the other 350 pipelines? What about the 500 queues?) 
|
I had 450 pipelines to clean up, so having the function clean them all up, not just the first 100, would be preferable. Does make the pipeline loading code a little less elegant; but given this is a cleanup test only, it's probably worth the tradeoff
|
Added a "cleanup queues" test as well - produced using AI, copying the pattern of pipelines - a bit quick and dirty, but given this is just test code and appears to work, perhaps good enough to ship? |
This functions similar to the cleanup pipelines test, but instead focuses on cluster queues used for testing that can be cleaned up
The cleanup orphaned pipelines test (that's not really a test, more a maintenance task we need to run every once in a while) needs some serious TLC. It uses the testcase struct as a concurrency controller, and its logic is generally really confusing. This PR updates the cleanup test to use more standard go idioms for concurrency control, generally makes it a bit easier to read, and updates it so that it also deletes orphaned cluster queues. It also updates go-buildkite to the latest version -- we were fairly seriously behind
phantomwhale
force-pushed
the
fix-parallel-calls
branch
from
3d7f03a to
7a125dd
Compare
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestCleanupOrphanedQueues(t *testing.T) { |
There was a problem hiding this comment.
So my PR (#792) added similar functionality to the CleanupOrphanedResources test, because we can't delete queues that have jobs in them, so there's a light dependency between cancelling jobs (which happens as part of pipeline cleanup) and deleting queues.
because run order is randomised, this means that sometimes this "test" will run in parallel with TestCleanupOrphanedResources, which might lead to contention. i think it's best that we keep all of the cleanup logic in one test so that we can control order more easily.
t.Parallel() was being called multiple times on the same parent test t. The testcase.Init() method calls t.Parallel(), and it was being called in a loop with the parent test, causing a panic to occur.
This moves the testcase initialization inside the t.Run callback so that Init() (and thus t.Parallel()) is called on each subtest's *testing.T instead of the parent test.
This also removes the unused sync.WaitGroup variable and import.