fix(securitycenter): issues related to flaky tests in SHA & ETD #4898
Conversation
product-auto-label
bot
added
api: securitycenter
![code-review-assist[bot]](https://avatars.githubusercontent.com/in/1008254?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hi team,
I'm currently reviewing this pull request and will post my detailed review in a few minutes. In the meantime, here's a quick summary to help you and other reviewers get up to speed:
This pull request addresses flaky tests in the golang-samples repository, specifically those related to Security Health Analytics (SHA) and Event Threat Detection (ETD). The core issue was a concurrent modification error causing test instability.
Here's a breakdown of the changes:
- Added retry mechanism: A
retryOperationfunction was implemented to handle transient errors during cleanup of existing custom modules. This improves the robustness of the test suite. - Added Mutex: A mutex (
mu sync.Mutex) was added to protect shared resources in the test functions, preventing race conditions and concurrent modification errors. This addresses the root cause of the flakiness. - Refactored cleanup functions: The
cleanupExistingCustomModulesandcleanupCustomModulefunctions were extracted and improved for better readability and maintainability. These functions now efficiently clean up test resources before and after test execution. - Renamed test functions: Several test functions were renamed for clarity and consistency (e.g.,
TestCreateCustomModuletoTestCreateSHACustomModule).
The changes are primarily focused on securitycenter/management_api/event_threat_detection/event_threat_detection_custom_module_test.go and securitycenter/management_api/security_health_analytics_custom_module_test.go. The line numbers in the diff clearly show the additions of the mutex, retry logic, and refactoring of the cleanup functions. Specific line changes include the addition of the sync package import (lines 26 in both files), the declaration of the mutex (lines 38 in both files), and the implementation of the retry logic (lines 48-52 in both files).
I'll provide a more detailed review shortly, but this summary should give you a good overview of the changes and their intent.
And now, a little haiku to lighten the mood:
Tests once flaky,
Now run smooth, a mutex's grace,
Code review's delight.
|
Gemini encountered an error creating the review. You can try again by commenting |
There was a problem hiding this comment.
Please use testutil.Retry instead of creating your own retry function.
https://github.com/GoogleCloudPlatform/golang-samples/blob/main/internal/testutil/retry.go
… github.com:vijaykanthm/golang-samples into fix-scc-mgt-api-sha-etd-custom-module-test-failures Merge remote into local
| parts := strings.Split(trimmedFullName, "/") | ||
| if len(parts) > 0 { | ||
| return parts[len(parts)-1] | ||
| func cleanupExistingCustomModules(orgID string) error { |
There was a problem hiding this comment.
issue!: Remove this. We sometimes have tests running in parallel--this could potentially delete a resource used by another test.
| @@ -225,10 +220,13 @@ func TestCreateEtdCustomModule(t *testing.T) { | |||
| func TestGetEtdCustomModule(t *testing.T) { | |||
| var buf bytes.Buffer | |||
|
|
|||
| mu.Lock() | |||
There was a problem hiding this comment.
question: why the mutex here? What shared resource is being protected inside the critical section / this function?
| @@ -254,10 +252,13 @@ func TestGetEtdCustomModule(t *testing.T) { | |||
| func TestUpdateEtdCustomModule(t *testing.T) { | |||
| var buf bytes.Buffer | |||
|
|
|||
| mu.Lock() | |||
| @@ -281,10 +282,13 @@ func TestUpdateEtdCustomModule(t *testing.T) { | |||
| func TestDeleteEtdCustomModule(t *testing.T) { | |||
| var buf bytes.Buffer | |||
|
|
|||
| mu.Lock() | |||
| @@ -308,10 +312,13 @@ func TestDeleteEtdCustomModule(t *testing.T) { | |||
| func TestListEtdCustomModule(t *testing.T) { | |||
| var buf bytes.Buffer | |||
|
|
|||
| mu.Lock() | |||
| @@ -336,10 +343,13 @@ func TestListEtdCustomModule(t *testing.T) { | |||
| func TestListEffectiveEtdCustomModule(t *testing.T) { | |||
| var buf bytes.Buffer | |||
|
|
|||
| mu.Lock() | |||
| @@ -364,10 +374,13 @@ func TestListEffectiveEtdCustomModule(t *testing.T) { | |||
| func TestGetEffectiveEtdCustomModule(t *testing.T) { | |||
| var buf bytes.Buffer | |||
|
|
|||
| mu.Lock() | |||
| @@ -393,10 +406,13 @@ func TestGetEffectiveEtdCustomModule(t *testing.T) { | |||
| func TestListDescendantEtdCustomModule(t *testing.T) { | |||
| var buf bytes.Buffer | |||
|
|
|||
| mu.Lock() | |||
| @@ -421,10 +437,13 @@ func TestListDescendantEtdCustomModule(t *testing.T) { | |||
| func TestValidateEtdCustomModule(t *testing.T) { | |||
| var buf bytes.Buffer | |||
|
|
|||
| mu.Lock() | |||
| parts := strings.Split(trimmedFullName, "/") | ||
| if len(parts) > 0 { | ||
| return parts[len(parts)-1] | ||
| func cleanupExistingCustomModules(orgID string) error { |
There was a problem hiding this comment.
See previous about deleting all resources in a organization.
|
I've looked through the logs for several of these failed builds, and it is only Create and Delete operations that lead to "concurrent modification" errors - To me, this points to the organization object being the contended write, not the module. To tackle that, i'd suggest creating a small number of modules in TestMain(), and reusing those modules throughout the tests. The current tests appear to create at least 15 modules. Another way to approach this is to use |
|
As per the email discussion, closing this PR, will refactor the clean up of resources to avoid the resource contention issue and will raise a new one. |
Description
Fixes #4866 #4871 Flakiness in tests due to Concurrent Modification error.
Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.
Checklist
go test -v ./..(see Testing)gofmt(see Formatting)go vet(see Formatting)