Skip to content
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

feat: draft: adds a onMissedRequest hook for seeing missed mocks #2682

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

James1x0
Copy link
Contributor

This relates to...

#2681

Rationale

To enable easier debugging of the mockagent, as discussed in #1600

Changes

  • docs: added an example of mockAgent onMissedRequest option
  • test: added a basic test for onMissedRequest
  • feat: added onMissedRequest
    • Added mock symbol for callback
    • Added logic in mock-utils dispatch to emit the hook

Features

  • added onMissedRequest hook for debugging mismatched requests

Bug Fixes

No bugs fixed

Breaking Changes and Deprecations

No breaking changes or deprecations

Status

  • I have read and agreed to the Developer's Certificate of Origin
  • Tested
  • [S] Benchmarked (optional)
  • Documented (will add specific option documentation in case spec changes)
  • Review ready
  • In review
  • Merge ready

@James1x0 James1x0 changed the title feat: adds a onMissedRequest hook for seeing missed mocks feat: draft: adds a onMissedRequest hook for seeing missed mocks Jan 31, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jan 31, 2024

Codecov Report

Attention: 208 lines in your changes are missing coverage. Please review.

Comparison is base (e39a632) 85.54% compared to head (a778077) 85.39%.
Report is 255 commits behind head on main.

Files Patch % Lines
lib/fetch/index.js 69.04% 52 Missing ⚠️
lib/fetch/util.js 48.45% 50 Missing ⚠️
lib/handler/RetryHandler.js 74.35% 30 Missing ⚠️
lib/cache/cache.js 12.12% 29 Missing ⚠️
lib/api/readable.js 83.92% 9 Missing ⚠️
lib/core/diagnostics.js 84.74% 9 Missing ⚠️
lib/eventsource/eventsource.js 96.09% 5 Missing ⚠️
lib/fetch/headers.js 90.38% 5 Missing ⚠️
lib/core/request.js 94.36% 4 Missing ⚠️
lib/client.js 95.83% 3 Missing ⚠️
... and 7 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2682      +/-   ##
==========================================
- Coverage   85.54%   85.39%   -0.16%     
==========================================
  Files          76       84       +8     
  Lines        6858     7544     +686     
==========================================
+ Hits         5867     6442     +575     
- Misses        991     1102     +111     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

```js
import { MockAgent } from 'undici'

const mockAgent = new MockAgent({ onMissedRequest: (error) => console.log(`A request wasn't handled: ${error.message}`) })
Copy link
Member

Choose a reason for hiding this comment

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

I like the idea pretty much, but not sure if it should be applied to the Agent. Have you considered the interceptor instead?

In that way, I imagine a usage like:

mockPool.intercept({
  path: '/justatest?hello=there&see=ya',
  method: 'GET'
}).reply(200, { wont: 'match' }, {
  headers: { 'content-type': 'application/json' }
// being response the object passed through the intercept
}).onNotMatched((request, response) => console.log(request, response))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I like this usage instead as well. Any potential downsides to this syntax?

Copy link
Member

Choose a reason for hiding this comment

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

I prefer this actually.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Could you pass the request that was missed to the callback?

@James1x0
Copy link
Contributor Author

James1x0 commented Feb 2, 2024

Could you pass the request that was missed to the callback?

@mcollina Good idea. We'd want both the request and the error, right? My argument for the error is that the reasoning is baked into the MockNotMatchedError throws.

Also - How do you feel about @metcoder95's alternative syntax?

@metcoder95
Copy link
Member

SGTM, feel free to add it 🚀

@mcollina
Copy link
Member

mcollina commented Feb 8, 2024

@James1x0 sorry for the delay. I prefer @metcoder95 syntax.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants