Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion lib/mock/snapshot-agent.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,9 @@ class SnapshotAgent extends MockAgent {
this[kSnapshotLoaded] = false

// For recording/update mode, we need a real agent to make actual requests
if (this[kSnapshotMode] === 'record' || this[kSnapshotMode] === 'update') {
// For playback mode, we need a real agent if there are excluded URLs
if (this[kSnapshotMode] === 'record' || this[kSnapshotMode] === 'update' ||
(this[kSnapshotMode] === 'playback' && opts.excludeUrls && opts.excludeUrls.length > 0)) {
this[kRealAgent] = new Agent(opts)
}

Expand All @@ -80,6 +82,20 @@ class SnapshotAgent extends MockAgent {
handler = WrapHandler.wrap(handler)
const mode = this[kSnapshotMode]

// Check if URL should be excluded (pass through without mocking/recording)
if (this[kSnapshotRecorder].isUrlExcluded(opts)) {
if (this[kRealAgent]) {
return this[kRealAgent].dispatch(opts, handler)
}
// If no real agent in playback mode, this is a configuration error
const error = new UndiciError('Cannot pass through excluded URL: no real agent available')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we should throw, maybe just go with no-op?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, my thought was since this would likely be because of misconfiguration, we would want to fail loudly rather than have the request swallowed. Thoughts?

Yeah I noticed those as well. My thinking was these were unrelated but can we confirm?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a configuration error, it will be better to throw at the moment the agent is being instantiated, as the configuration mismatch should be identified and advertised early, usually when at runtime is already late to take further action.

If we can fail early, the better, otherwise a warning will be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some constructor logic in an earlier commit ensuring an agent is created if needed so we should be good. Removed the error block

if (handler.onError) {
handler.onError(error)
return
}
throw error
}

if (mode === 'playback' || mode === 'update') {
// Ensure snapshots are loaded
if (!this[kSnapshotLoaded]) {
Expand Down
16 changes: 12 additions & 4 deletions lib/mock/snapshot-recorder.js
Original file line number Diff line number Diff line change
Expand Up @@ -283,8 +283,7 @@ class SnapshotRecorder {
}

// Check URL exclusion patterns
const url = new URL(requestOpts.path, requestOpts.origin).toString()
if (this.#isUrlExcluded(url)) {
if (this.isUrlExcluded(requestOpts)) {
return // Skip recording
}

Expand Down Expand Up @@ -330,6 +329,16 @@ class SnapshotRecorder {
}
}

/**
* Checks if a URL should be excluded from recording/playback
* @param {SnapshotRequestOptions} requestOpts - Request options to check
* @returns {boolean} - True if URL is excluded
*/
isUrlExcluded (requestOpts) {
const url = new URL(requestOpts.path, requestOpts.origin).toString()
return this.#isUrlExcluded(url)
}

/**
* Finds a matching snapshot for the given request
* Returns the appropriate response based on call count for sequential responses
Expand All @@ -344,8 +353,7 @@ class SnapshotRecorder {
}

// Check URL exclusion patterns
const url = new URL(requestOpts.path, requestOpts.origin).toString()
if (this.#isUrlExcluded(url)) {
if (this.isUrlExcluded(requestOpts)) {
return undefined // Skip playback
}

Expand Down
57 changes: 57 additions & 0 deletions test/snapshot-testing.js
Original file line number Diff line number Diff line change
Expand Up @@ -1293,6 +1293,63 @@ describe('SnapshotAgent - Filtering', () => {
assert.strictEqual(snapshots[0].request.method, 'GET',
'Recorded request should have GET method')
})

it('excluded URLs should not error in playback mode', async (t) => {
const server = createTestServer((req, res) => {
res.writeHead(200, { 'content-type': 'text/plain' })
res.end(`Response from ${req.url}`)
})

const { origin } = await setupServer(server)
const snapshotPath = createSnapshotPath('exclude-playback-bug')

setupCleanup(t, { server, snapshotPath })

// Record mode: record one request, exclude another
const recordingAgent = new SnapshotAgent({
mode: 'record',
snapshotPath,
excludeUrls: [`${origin}/excluded`]
})

const originalDispatcher = getGlobalDispatcher()
setupCleanup(t, { agent: recordingAgent, originalDispatcher })
setGlobalDispatcher(recordingAgent)

// Request to included endpoint - should be recorded
const res1 = await request(`${origin}/included`)
await res1.body.text()

// Request to excluded endpoint - should NOT be recorded
const res2 = await request(`${origin}/excluded`)
await res2.body.text()

await recordingAgent.saveSnapshots()

const recorder = recordingAgent.getRecorder()
assert.strictEqual(recorder.size(), 1, 'Should have recorded only the included request')

// Playback mode: should allow excluded URL to pass through without error
const playbackAgent = new SnapshotAgent({
mode: 'playback',
snapshotPath,
excludeUrls: [`${origin}/excluded`]
})

setupCleanup(t, { agent: playbackAgent })
setGlobalDispatcher(playbackAgent)

// This should work - replays from snapshot
const res3 = await request(`${origin}/included`)
await res3.body.text()
assert.strictEqual(res3.statusCode, 200, 'Included request should replay successfully')

// Excluded URL should pass through to real server
const res4 = await request(`${origin}/excluded`)
const body4 = await res4.body.text()
assert.strictEqual(res4.statusCode, 200, 'Excluded request should pass through to real server')
assert.strictEqual(body4, 'Response from /excluded', 'Should get live response from server')
})
})

describe('SnapshotAgent - Close Method', () => {
Expand Down
Loading