-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(securitycenter): issues related to flaky tests in SHA & ETD #5090
base: main
Are you sure you want to change the base?
fix(securitycenter): issues related to flaky tests in SHA & ETD #5090
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi team,
I'm currently reviewing this pull request and will provide my feedback in a few minutes. In the meantime, here's a summary of the changes for you and other reviewers to quickly get up to speed:
This pull request, authored by vijaykanthm, aims to fix flaky tests in the golang-samples
repository related to Security Health Analytics (SHA) and Event Threat Detection (ETD). The root cause was identified as a concurrent modification error.
Here's a breakdown of the changes:
-
Improved Uniqueness in Custom Module Creation: The code now uses the
google/uuid
package to generate unique suffixes for custom module display names. This eliminates the previous reliance onmath/rand
andtime.Now()
, which could lead to collisions and flaky tests. Specifically:- In
securitycenter/management_api/create_security_health_analytics_custom_module.go
, lines 45-49 were modified to use UUIDs and a regular expression to sanitize the generated suffix for valid characters. The old random number generation was removed (lines 23, 24). - Similar changes were made in
securitycenter/management_api/event_threat_detection/create_event_threat_detection_custom_module.go
, lines 41-44 were updated to use UUIDs. The old random number generation was removed (lines 23, 24). - The test file
securitycenter/management_api/event_threat_detection/event_threat_detection_custom_module_test.go
was significantly refactored to improve test stability and cleanup. The old random number generation was removed (lines 22, 26). New functions were added to manage cleanup of created modules:AddModuleToCleanup
,PrintAllCreatedModules
,CleanupCreatedModules
,CustomModuleExists
, andCleanupAfterTests
. ThecleanupCustomModule
andcleanupExistingCustomModules
functions were removed. - Similar changes were made in
securitycenter/management_api/security_health_analytics_custom_module_test.go
, lines 22-34 were modified to use UUIDs and a regular expression to sanitize the generated suffix for valid characters. The old random number generation was removed (lines 22, 26). New functions were added to manage cleanup of created modules:AddModuleToCleanup
,PrintAllCreatedModules
,CleanupCreatedModules
,CustomModuleExists
, andCleanupAfterTests
. ThecleanupCustomModule
andcleanupExistingCustomModules
functions were removed.
- In
-
Enhanced Test Stability: The test suite now includes robust cleanup mechanisms to ensure that test runs don't interfere with each other. This addresses the flakiness caused by concurrent modifications.
-
Improved Error Handling: The code now includes more comprehensive error handling to improve the overall robustness of the application.
The pull request addresses issues #4866 and #4871.
I'll post my detailed review shortly.
Meanwhile, here's a little poem for your enjoyment:
Tests once flaky, now run true,
UUIDs banish errors new.
Clean code sings a joyful tune,
Review complete, by afternoon!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR introduces a good fix for the flaky tests by using UUIDs and cleaning up the created custom modules after tests. The implementation is well-structured and addresses the concurrency issues effectively. However, there are a few minor improvements that can be made.
// Remove invalid characters (anything that isn't alphanumeric or an underscore) | ||
re := regexp.MustCompile(`[^a-zA-Z0-9_]`) | ||
uniqueSuffix = re.ReplaceAllString(uniqueSuffix, "_") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rand.Seed(time.Now().UnixNano()) | ||
// Generate a unique suffix | ||
uniqueSuffix := fmt.Sprintf("%d-%d", time.Now().Unix(), rand.Intn(1000)) | ||
uniqueSuffix := uuid.New().String() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding logic to handle potential UUID collisions, similar to what's done in create_security_health_analytics_custom_module.go
.
44: uniqueSuffix := uuid.New().String()
45:
46: // Remove invalid characters (anything that isn't alphanumeric or an underscore)
47: re := regexp.MustCompile(`[^a-zA-Z0-9_]`)
48: uniqueSuffix = re.ReplaceAllString(uniqueSuffix, "_")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed.
|
||
parent := fmt.Sprintf("organizations/%s/locations/global", orgID) | ||
|
||
// Call Create | ||
err = createEventThreatDetectionCustomModule(&buf, parent) | ||
err := createEventThreatDetectionCustomModule(&buf, parent) | ||
|
||
if err != nil { | ||
t.Fatalf("createCustomModule() had error: %v", err) | ||
return | ||
} | ||
|
||
got := buf.String() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The createModulePath
might be empty if the output format changes. Add a check to handle this case gracefully.
230: got := buf.String()
231: if got == "" {
232: t.Errorf("createEventThreatDetectionCustomModule() returned an empty string")
233: return
234: }
235: fmt.Printf("Response: %v\n", got)
236:
237: parts := strings.Split(got, ":")
238: if len(parts) > 0 {
239: createModulePath = parts[len(parts)-1]
240: }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed.
@@ -242,6 +271,14 @@ func TestCreateCustomModule(t *testing.T) { | |||
} | |||
|
|||
got := buf.String() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The createModulePath
might be empty if the output format changes. Add a check to handle this case gracefully.
273: got := buf.String()
274: if got == "" {
275: t.Errorf("createSecurityHealthAnalyticsCustomModule() returned an empty string")
276: return
277: }
278: fmt.Printf("Response: %v\n", got)
279:
280: parts := strings.Split(got, ":")
281: if len(parts) > 0 {
282: createModulePath = parts[len(parts)-1]
283: }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed.
…ithub.com:vijaykanthm/golang-samples into fix-scc-mgt-api-sha-etd-cust-mod-test-flaky-issue Merge remote into local
"os" | ||
"strings" | ||
// "sync" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: remove commented-out code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed.
"google.golang.org/protobuf/types/known/structpb" | ||
) | ||
|
||
var orgID = "" | ||
var createdCustomModuleID = "" | ||
var ( | ||
createdModules []string | ||
// mu sync.Mutex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: remove commented-out code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed.
"strings" | ||
// "sync" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: remove commented out code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed.
) | ||
|
||
var orgID = "" | ||
var createdCustomModuleID = "" | ||
var ( | ||
createdModules []string | ||
// mu sync.Mutex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: remove commented out code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed.
…ithub.com:vijaykanthm/golang-samples into fix-scc-mgt-api-sha-etd-cust-mod-test-flaky-issue merge remote to local
…ithub.com:vijaykanthm/golang-samples into fix-scc-mgt-api-sha-etd-cust-mod-test-flaky-issue Merge remote into local
Description
Fixes #4866, #4871 Flakiness in tests due to Concurrent Modification error by refactoring the clean up process of created modules for testing.
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.
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)