-
Notifications
You must be signed in to change notification settings - Fork 265
Stabilize integration tests by using a single shared mock server instead of multiple per-test servers #870
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: KaveeshaPiumini <[email protected]>
…prove comments Co-authored-by: KaveeshaPiumini <[email protected]>
…erver state The shared mock server implementation was causing race conditions where users, auth codes, and tokens from one test suite would persist when another test suite ran. This led to non-deterministic behavior since Go maps iterate in random order, causing the mock server to randomly select different users. Changes: - Added Reset() method to all mock servers (Google, GitHub, OAuth, OIDC) to clear users, auth codes, tokens, and custom authorize functions - Updated all test suites to call Reset() in SetupSuite() before adding test users, ensuring clean state for each test suite - This eliminates race conditions and makes tests deterministic Affected test suites: - GoogleAuthFlowTestSuite - ConditionalExecAuthFlowTestSuite - GoogleRegistrationFlowTestSuite - GithubAuthFlowTestSuite - GithubRegistrationFlowTestSuite - OAuthAuthTestSuite - OIDCAuthTestSuite Fixes intermittent failures in TestGoogleAuthFlowCompleteSuccess and TestGoogleAuthFlowMultipleUsersSuccess among others.
8839e50 to
89528a8
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #870 +/- ##
===========================================
+ Coverage 62.07% 88.25% +26.18%
===========================================
Files 322 324 +2
Lines 26441 26446 +5
Branches 606 606
===========================================
+ Hits 16412 23340 +6928
+ Misses 7593 1933 -5660
+ Partials 2436 1173 -1263
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Add WaitForMessage method to MockNotificationServer that polls for messages with a configurable timeout instead of using fixed sleep - Replace time.Sleep + GetLastMessage calls with WaitForMessage in SMS registration tests for more reliable test execution - This addresses intermittent test failures caused by timing issues when waiting for SMS OTP messages
| ts.testAppID = appID | ||
|
|
||
| // Start mock notification server | ||
| ts.mockServer = testutils.NewMockNotificationServer(mockNotificationServerPort) |
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.
Shall we change this constructor to private newMockNotificationServer. So we can make sure mock server will be always retrieved from testutils.GetSharedMockServers() in future.
Let's do this for all the mock server constructor in the test_utils package
|
|
||
| // Stop mock server | ||
| if ts.mockServer != nil { | ||
| err := ts.mockServer.Stop() |
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.
Sameway, we can make the stop package private. So other package can't invoke it.
Let's do this for all the mock server constructor in the test_utils package
| time.Sleep(500 * time.Millisecond) | ||
|
|
||
| // Verify SMS was sent | ||
| lastMessage := ts.mockServer.GetLastMessage() |
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.
Sameway, we can make the getLastMessage package private. So other package can't invoke it.
Let's do this for all the mock server constructor in the test_utils package
Purpose
Integration tests were failing intermittently due to race conditions when multiple test suites started/stopped their own mock servers on the same ports (8092, 8093, 8098, 9091). When teardown didn't complete before the next suite's setup, ports were left in inconsistent states.
Approach
Introduced a singleton
SharedMockServersmanager that starts each mock server once and shares it across all test suites.Key changes:
testutils/shared_mock_servers.gowith thread-safe singleton patternGetSharedMockServers().Get*Server()instead of creating/starting their ownStart()/Stop()calls fromSetupSuite()/TearDownSuite()Usage:
Updated test files:
flowauthn/: sms_auth, decision_auth, http_request_executor_auth, github_auth, google_auth, conditional_exec_authflowregistration/: sms_registration, ou_registration, http_request_executor_registration, github_registration, google_registrationauthn/: oauth_auth, oidc_authRelated Issues
Related PRs
Checklist
Security checks
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.