-
Notifications
You must be signed in to change notification settings - Fork 925
GODRIVER-3517: Convert OIDC prose test to go test #2283
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
GODRIVER-3517: Convert OIDC prose test to go test #2283
Conversation
This move is since the new test won't be primarily run by a go build then run, instead it will use the go testing framework
Follow design patterns from other prose tests Update functions to use testing framework Update test main to allow for compiling to use the test the way it currently is used remove uneeded var rename Keep the diff change as small as required for reviews
94daf67 to
81f357e
Compare
🧪 Performance ResultsCommit SHA: 667c71dThe following benchmark tests for version 697280a49e5ee4000720b7f4 had statistically significant changes (i.e., |z-score| > 1.96):
For a comprehensive view of all microbenchmark results for this PR's commit, please check out the Evergreen perf task for this patch. |
API Change ReportNo changes found! |
When converting this test to a go test it is now picked up by many variants on evergreen that run go test ./... This is an issue because the test requires env vars This change skips it except for the cases where it is called with required env vars
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.
Pull request overview
This PR converts OIDC prose tests from a custom main package with manual test execution to standard Go testing framework. The changes move the test from internal/cmd to internal/test and refactor all test functions to use standard testing.T patterns while maintaining the ability to compile as a binary for remote execution.
- Changed package from
maintooidcauthand added properTestMainfor environment-based test skipping - Converted all test functions from
func name() errortofunc TestName(t *testing.T)with proper test naming conventions - Updated test infrastructure (shell scripts and Taskfile) to use
go testcommands instead ofgo run
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/test/oidcauth/oidcauth_test.go | Main test file converted from custom main package to standard Go test package with ~50 test functions refactored to use testing.T, proper cleanup patterns, and environment-based conditional execution |
| etc/run-oidc-remote-test.sh | Updated to compile tests with go test -c instead of go build, and added -test.v flag for verbose output |
| Taskfile.yml | Updated OIDC test commands from go run to go test -v with proper test path patterns |
Comments suppressed due to low confidence (1)
internal/test/oidcauth/oidcauth_test.go:1545
- The error message "expected 1 finds succeed" is grammatically incorrect. It should be "expected 1 find to succeed" or "expected 1 find succeeded".
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if err != nil { | ||
| return fmt.Errorf("machine_1_1: failed connecting client: %v", err) | ||
| t.Fatalf("failed connecting client: %v", err) |
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.
[blocking] We should update all tests to use the internal require package instead of if blocks.
Use the internal require package to check test values Reduces lines of code and makes the intention of the checks more clear
|
I've reconfigured the Evergreen test variants to run the "OIDC" tests, which are not run by default on PRs. We will have to do that for each new commit in this PR. |
|
@matthewdale Thanks for clearing that up. I added them to the current commit's patch https://spruce.mongodb.com/version/69680ef363809200070b3c99/tasks?page=0&sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC&variant=%5Etestoidc-variant%24 |
Accidentally inversed the check when converting to the require package
| require.True( | ||
| t, | ||
| opts.Auth == nil || opts.Auth.AuthMechanism != "MONGODB-OIDC", | ||
| "expected URI to contain MONGODB-OIDC auth information", | ||
| ) |
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.
[blocking] The logic here is inverted from the original assertion.
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.
Thanks good catch I missed this one, just pushed the change
Co-authored-by: Preston Vasquez <[email protected]>
Co-authored-by: Preston Vasquez <[email protected]>
Co-authored-by: Preston Vasquez <[email protected]>
|
@prestonvasquez changes are up to date on comments now. Ran the OIDC tests in the patch they pass but I'm noticing an issue. Because the Azure, GCP, and K8s tests run on another platform the test output and the benefits of the go tests like listing the tests is not available. Is there a fix for this? And is there a case where those tests fail but evergreen won't know or would it know it fails but it would be unable to list which test failed? https://spruce.mongodb.com/version/697144ad2fc1e400073afe6c/tasks?page=0&sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC&variant=%5Etestoidc-variant%24 |
@RafaelCenzano Need to redirect (e.g. |
| # Ensure that we source the environment file created for us, set up any other variables we need, | ||
| # and then run our test suite on the vm. | ||
| export AZUREOIDC_TEST_CMD="PROJECT_DIRECTORY='.' OIDC_ENV=azure OIDC=oidc ./etc/run-oidc-test.sh ./test" | ||
| export AZUREOIDC_TEST_CMD="PROJECT_DIRECTORY='.' OIDC_ENV=azure OIDC=oidc ./etc/run-oidc-test.sh ./test -test.v" |
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.
[blocking] See #2283 (comment)
|
Most recent OIDC run for the most recent changes. https://spruce.mongodb.com/version/697280a49e5ee4000720b7f4/tasks?page=0&sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC&variant=%5Etestoidc-variant%24 |
GODRIVER-3517
Summary
Background & Motivation
The current code in master uses custom code to validate results and doesn't output in a standard format making it hard to view and read in CI/CD. By switching to using the go testing package things will be standardized and the same results can achieved as the original test with less code.
Patches
From master branch: https://spruce.mongodb.com/task/mongo_go_driver_testoidc_variant_oidc_auth_test_patch_d6ad23466699cd643cac9f2bfc4039dc8b90e4fd_69604bcc0446b80007af76b1_26_01_09_00_29_52/logs?execution=0
From the new changes: https://spruce.mongodb.com/task/mongo_go_driver_testoidc_variant_oidc_auth_test_patch_2d42254f49b16346c9a642093bd5283f35f0e6ac_696048178201660007bf7900_26_01_09_00_13_59/logs?execution=0