test: standardize runtime.ParseFeatures error handling and mutex usage#4416
Conversation
|
Build Succeeded 🥳 Build Id: 636ddd9a-cf4f-4b78-8625-efa54c88bf21 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version: |
| defer func() { | ||
| _ = utilruntime.ParseFeatures("") | ||
| require.NoError(t, utilruntime.ParseFeatures("")) |
There was a problem hiding this comment.
Is this needed? None of the other tests use a cleanup function for the ParseFeatures.
If we do need the cleanup function, should this be assert instead of require? The require inside of a defer cleanup function might mask a primary failure elsewhere.
There was a problem hiding this comment.
Agreed - it was there when I was refactoring. Happy to remove.
| runtime.FeatureTestMutex.Lock() | ||
| defer runtime.FeatureTestMutex.Unlock() | ||
|
|
There was a problem hiding this comment.
The mutex forces this test to run serially as it waits for the global lock, even though there's t.Parallel, correct?
As an alternative, should we remove t.Parallel from all tests that change the state with ParseFeatures or otherwise, and run all other tests that do not need to change the state with t.Parallel? That way all the tests that need to lock run serially, and all remaining tests are run in parallel with and only with other Parallel tests.
There was a problem hiding this comment.
The benefit of doing it this way is that other non-mutex'd tests can run in parallel with this one - so at least we can get some speed benefits of those running in parallel together.
This also gives us some testing of changes across unit tests of different feature gates -- but it can lead to flaky tests -- but it can find some potential issues around the way.
There was a problem hiding this comment.
It would be interesting to see what the speed difference would be across all e2e tests if we made the change. Not needed for this PR though :)
| if !assert.Equal(t, http.StatusCreated, rec.Code, "Unexpected HTTP Code") { | ||
| require.Failf(t, "Unexpected HTTP Code Body: %s", rec.Body.String()) | ||
| } |
There was a problem hiding this comment.
Nit:
| if !assert.Equal(t, http.StatusCreated, rec.Code, "Unexpected HTTP Code") { | |
| require.Failf(t, "Unexpected HTTP Code Body: %s", rec.Body.String()) | |
| } | |
| require.Equal(t, http.StatusCreated, rec.Code, "Response body: %s", rec.Body.String()) |
There was a problem hiding this comment.
Oh yeah - that makes way more sense. 🤦🏻
Ensure all calls to runtime.ParseFeatures in unit tests follow consistent patterns: - Wrap all calls with require.NoError instead of assert.NoError to fail fast on parse errors - Add runtime.FeatureTestMutex locks with defer unlocks to all tests/subtests using ParseFeatures - Fix mutex scope in pkg/sdkserver/sdk_test.go by moving locks from parent test to individual subtests This ensures thread-safe feature flag manipulation and prevents test flakiness from concurrent feature state modifications. This also includes some (probable) fixes for flaky tests, particularly closes agones-dev#4407 as part of this work.
f7f6c87 to
e9d348b
Compare
|
Build Succeeded 🥳 Build Id: 78265f6b-4e3b-4479-9f46-8bfe03ffef77 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version: |
|
Build Failed 😭 Build Id: 04b5cae1-eba4-4175-a142-eb8e8a60f986 Status: FAILURE To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
|
On mobile atm, but there's a link that's not working anymore. |
|
Build Succeeded 🥳 Build Id: b5a72048-4301-4599-a652-4ab794d2e9bf The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version: |
agones-dev#4416) * test: standardize runtime.ParseFeatures error handling and mutex usage Ensure all calls to runtime.ParseFeatures in unit tests follow consistent patterns: - Wrap all calls with require.NoError instead of assert.NoError to fail fast on parse errors - Add runtime.FeatureTestMutex locks with defer unlocks to all tests/subtests using ParseFeatures - Fix mutex scope in pkg/sdkserver/sdk_test.go by moving locks from parent test to individual subtests This ensures thread-safe feature flag manipulation and prevents test flakiness from concurrent feature state modifications. This also includes some (probable) fixes for flaky tests, particularly closes agones-dev#4407 as part of this work. * Review updates.
What type of PR is this?
/kind cleanup
What this PR does / Why we need it:
Ensure all calls to runtime.ParseFeatures in unit tests follow consistent patterns:
Wrap all calls with require.NoError instead of assert.NoError to fail fast on parse errors
Add runtime.FeatureTestMutex locks with defer unlocks to all tests/subtests using ParseFeatures
Fix mutex scope in pkg/sdkserver/sdk_test.go by moving locks from parent test to individual subtests
This ensures thread-safe feature flag manipulation and prevents test flakiness from concurrent feature state modifications.
Which issue(s) this PR fixes:
This also includes some (probable) fixes for flaky tests, particularly closes #4407 as part of this work.
Special notes for your reviewer:
🤞🏻