test(NODE-7426): Add appName to OIDC test failpoints#4869
test(NODE-7426): Add appName to OIDC test failpoints#4869dariakp merged 6 commits intomongodb:mainfrom
Conversation
PavelSafronov
force-pushed
the
NODE-7426
branch
from
7cdb6ac to
553631d
Compare
dariakp
added
the
Primary Review
- configured a distinct per-test appName - removed generated appName
There was a problem hiding this comment.
Pull request overview
Updates OIDC authentication tests to scope server failpoints by appName (per updated OIDC spec guidance) and to explicitly clean up failpoints between test runs, reducing cross-test interference.
Changes:
- Add
serverless: forbidto the unified OIDC no-retry spec test. - Configure a stable
appNamein the unified spec test client and includeappNamein failpointdata. - Update OIDC prose integration tests to generate unique
appNamevalues per describe block, pass them to clients, and attempt explicit failpoint teardown.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 17 comments.
| File | Description |
|---|---|
| test/spec/auth/unified/mongodb-oidc-no-retry.yml | Adds serverless: forbid, introduces appName anchor, and scopes failpoints via appName. |
| test/spec/auth/unified/mongodb-oidc-no-retry.json | JSON equivalent of the unified spec updates (serverless forbid, appName, appName-scoped failpoints). |
| test/integration/auth/mongodb_oidc.prose.test.ts | Adds per-test appName generation, passes appName into clients and failpoints, and adds explicit failpoint cleanup (with issues noted in PR comments). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- typos fixed - appName added in spots it was missing
|
@tadjik1 FYI Copilot concerns addressed (most of the comments are wrong about appName), tests are passing. |
| client = getClient({}, callbackSpy); | ||
| utilClient = getClient({}, createCallback()); | ||
| client = getClient({ appName }, callbackSpy); | ||
| utilClient = getClient({ appName }, createCallback()); |
There was a problem hiding this comment.
I think we only want to set the appName on the client and not the utilClient, since the utilClient isn't the client under test. @nbbeeken can you check me on this?
There was a problem hiding this comment.
Yep, the appname is the thing the server is using to apply the failcommand or not, every connection to MongoDB starts with a hello that includes the client metadata (thus the appname). So that's how the failcommand gets filtered properly
There was a problem hiding this comment.
Makes sense, the test client must have the appName applied to it, while the util client needs to pass the appName to the various configureFailPoint commands. Updated.
dariakp
added
Team Review
|
|
||
| describe('3.3 Unexpected error code does not clear the cache', function () { | ||
| let utilClient: MongoClient; | ||
| const appName = createAppName(); |
There was a problem hiding this comment.
Would it be better to assign appName inside the beforeEach? I know right now we have a single it block per describe, so it's basically the same thing, but if we were ever to expand these, it could lead to the appName being shared between 2 it-blocks; if we're truly aiming to have a unique name per created test client, we should capture that intention by putting the logic inside the the beforeEach where the clients themselves are created
There was a problem hiding this comment.
Absolutely. We should be recreating the app name every time we create a new client. Updated.
Description
Summary of Changes
Updated OIDC test specs.
Double-checked that failpoints are being removed
Updated UnifiedMongoClient to always configure an
appName, as either a passed-in value or a unique identifier, as called out for OIDC tests in mongodb/specifications@4d11668Double check the following
npm run check:lint)type(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript