fix: snapshot url exclusion #4670
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4670 +/- ##
==========================================
- Coverage 92.86% 92.84% -0.02%
==========================================
Files 107 107
Lines 33499 33515 +16
==========================================
+ Hits 31108 31117 +9
- Misses 2391 2398 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lib/mock/snapshot-agent.js
Outdated
| 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') |
There was a problem hiding this comment.
I'm not sure we should throw, maybe just go with no-op?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Added some constructor logic in an earlier commit ensuring an agent is created if needed so we should be good. Removed the error block
|
tests are still not happy |
|
@mcollina from what I can see, the remaining tests seem unrelated |
4 participants
Rationale
Addresses #4663
Changes/Fixes
SnapshotAgent.dispatch()excludeUrlsis configuredSnapshotRecorder.isUrlExcluded()for early URL exclusion checksStatus