feat: add pending interceptor check functions to mock agent #1358

theneva · 2022-04-21T21:44:59Z

Hi! 👋

This adds functions to check any pending interceptors on a mock agent, as well as a function that throws with a friendly, emoji-ful error message if there are any pending interceptors.

I think this closes #1002

I've never contributed here before, so please let me know if I should change anything! 🙌

(Huge thanks to @SimenB for helping me port it from our own repo)

mcollina

Good work!!!

lib/mock/mock-agent.js

lib/mock/table-formatter.js

theneva · 2022-04-22T08:58:18Z

Thanks for the review! I'm aware of some other nits and I'll address the review comments soon :)

mcollina · 2022-04-22T09:07:10Z

Also CI is failing

theneva · 2022-04-22T09:13:49Z

Yep, I think CI fails because I drop ansi colouring in the output if process.env.CI is set. Gonna take a look!

(Is reading that env var okay, or should I add a separate one? As an unfortunate Google Cloud Build user, I really appreciate it when tools default to no colours on CI.)

codecov-commenter · 2022-04-22T12:11:45Z

Codecov Report

Merging #1358 (6af598d) into main (a8c2918) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1358      +/-   ##
==========================================
+ Coverage   94.25%   94.29%   +0.03%     
==========================================
  Files          45       47       +2     
  Lines        4211     4240      +29     
==========================================
+ Hits         3969     3998      +29     
  Misses        242      242

Impacted Files	Coverage Δ
lib/mock/mock-agent.js	`100.00% <100.00%> (ø)`
lib/mock/mock-utils.js	`100.00% <100.00%> (ø)`
lib/mock/pending-interceptors-formatter.js	`100.00% <100.00%> (ø)`
lib/mock/pluralizer.js	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8c2918...6af598d. Read the comment docs.

theneva · 2022-04-22T12:39:08Z

I think it should be good now, but the linter will trip until #1361 lands I think

mcollina · 2022-04-23T12:08:08Z

Can you rebase? The linter landed

theneva · 2022-04-23T12:19:40Z

I merged main into my branch :) everything passed locally, so hopefully it all works now

mcollina

lgtm

@SimenB could you take a look?

SimenB

This nicely covers the use case I had in mind when opening the issue. Thanks! 👍

docs/api/MockAgent.md

lib/mock/mock-agent.js

SimenB · 2022-04-24T07:08:43Z

test/mock-interceptor-unused-assertions.js

@@ -0,0 +1,138 @@
+'use strict'


there are no tests for persist, can you add?

Also, I think a persisted interceptor should error if it's never called (I don't know if the logic does that today)

there are no tests for persist, can you add?

Yep!

Also, I think a persisted interceptor should error if it's never called (I don't know if the logic does that today)

I don't think that's currently possible with the way Undici counts requests, since the times value is only set if times(<number>) has been called. In this PR I "cheat" by treating times as 1 if neither persist nor times is set.

It could definitely be solved by tracking the call count counting upwards to either a limit set by times(), 1 if neither persist nor times is set, or forever if persist is set, before it stops serving the mocked response.

I can give it a try, but that feels like it might belong in a separate pull request. On the other hand, it would trip on a new assertion and thus be a breaking change to the API introduced in this PR, so maybe it belongs here…

Just so I don't waste time on code that will be rejected: @mcollina or @ronag are you open to me changing the call count tracking in this PR so it can all land at once?

Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>

SimenB

nice!

theneva · 2022-04-25T14:32:53Z

Okay, @SimenB and I sat down together, and greatly simplified the changeset.

Feature-wise, the reporter now:

Reports invocation count (timesInvoked) and expected number of invocations
No longer attempts to report counts for consumed interceptors (this can't be done consistently)
Trips on persistent interceptors that have never been invoked
Reports the Origin of pending interceptors

We also switched from calling interceptors "(un)consumed", and instead use "pending". consumed and pending are mostly opposites, with one exception: A persistent interceptor that has been invoked at least once is pending, but not consumed.

This came with more tests and updated docs. It'd be great if you took another look 😄

Edit: Oh, and we renamed the TableFormatter thing to PendingInterceptorsFormatter and pass it an array of PendingInterceptors instead of just treating it like console#table. That should make it a whole lot easier to build other reporters, and I think it made things less confusing.

theneva · 2022-04-25T14:35:48Z

lib/mock/mock-utils.js

@@ -107,9 +107,9 @@ function getMockDispatch (mockDispatches, key) {
 }

 function addMockDispatch (mockDispatches, key, data) {
-  const baseData = { times: null, persist: false, consumed: false }
+  const baseData = { timesInvoked: 0, times: 1, persist: false, consumed: false }


we default times to 1 here so we don't have to do null checking everywhere

theneva · 2022-04-25T14:37:30Z

lib/mock/mock-utils.js

  const replyData = typeof data === 'function' ? { callback: data } : { ...data }
-  const newMockDispatch = { ...baseData, ...key, data: { error: null, ...replyData } }
+  const newMockDispatch = { ...baseData, ...key, pending: true, data: { error: null, ...replyData } }


All interceptors are pending as they are first registered. I think consumed belongs here as well (and not in baseData), but… meh. I suppose it might be possible to register an interceptor that should not be pending by registering it with .times(0), but that sounds too illogical to account for.

theneva · 2022-04-25T14:38:13Z

lib/mock/mock-utils.js

@@ -230,25 +230,20 @@ function mockDispatch (opts, handler) {
  const key = buildKey(opts)
  const mockDispatch = getMockDispatch(this[kDispatches], key)

+  mockDispatch.timesInvoked++


We now increase timesInvoked instead of decreasing times (which is now constant).

(Is invoked a good name, or would something like timesIntercepted be better?)

theneva · 2022-04-25T14:38:42Z

lib/mock/mock-utils.js

-  }
+  // If it's used up and not persistent, mark as consumed
+  mockDispatch.consumed = !persist && timesInvoked >= times
+  mockDispatch.pending = timesInvoked < times


consumed is the same as before, but pending is new.

theneva · 2022-04-25T14:39:53Z

lib/mock/pending-interceptors-formatter.js

+        Persistent: persist ? '✅' : '❌',
+        Invocations: timesInvoked,
+        Remaining: persist ? Infinity : times - timesInvoked
+      }))


we moved the header name mapping here so that the formatter gets proper lowerCamelCased variable names passed in. I think that makes it less annoying to build a new formatter. Otherwise, this does the same thing as before

theneva · 2022-04-25T14:40:17Z

test/mock-interceptor-unused-assertions.js

@@ -0,0 +1,138 @@
+'use strict'


theneva · 2022-04-25T14:42:17Z

lib/mock/mock-agent.js

+      return
+    }
+
+    const pluralizer = new Pluralizer('interceptor', 'interceptors').pluralize(pending.length)


the pluralizer API ends up being a little more complicated than it maybe needs to now that we only track one array (instead of consumed, tooFewUses, and persistent as we did before), but I think it's still a nice little utility.

mcollina

lgtm

SimenB · 2022-04-25T20:17:31Z

Okay, @SimenB and I sat down together

Exclude my name from the commit, this is 99.99% @theneva. I just shouted encouragement from the back seat! 😀

That said, this PR is an awesome implementation of my feature request! ♥️

theneva · 2022-04-29T16:30:12Z

Awesome! Is there anything else I can do to help this land? 😄

SimenB · 2022-04-30T07:54:12Z

Yay!

SimenB · 2022-05-02T07:03:26Z

Testing this out, works great 👍

not sure if [Function: path] should be attempted to prettify somehow? Looks fine to me, and not sure of any other way it could be better.

Only thing I can think of that might make this even better is to capture an Error for every intercept call, to have a stack trace back to where the interceptor was registered.

)

mcollina reviewed Apr 22, 2022

View reviewed changes

lib/mock/mock-agent.js Outdated Show resolved Hide resolved

lib/mock/table-formatter.js Outdated Show resolved Hide resolved

mcollina approved these changes Apr 23, 2022

View reviewed changes

SimenB reviewed Apr 24, 2022

View reviewed changes

ronag approved these changes Apr 24, 2022

View reviewed changes

theneva force-pushed the unused-mock-interceptor branch from 59d4abd to 11ff74f Compare April 25, 2022 13:18

theneva and others added 10 commits April 25, 2022 16:11

feat: add pending interceptor check functions to mock agent

00dd6c5

fix: cleanup & various fixes

e2419f7

fix: don't use null coalescing

becb271

Update docs/api/MockAgent.md

bdab4bc

Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>

Update lib/mock/mock-agent.js

b8c8a76

Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>

don't return unnecessarily

adfb8e3

add tests for various permutations

b8c1a05

cover case where everything is fine

65bcea9

add test for all interceptors consumed

14517ab

simplify & improve things

6af598d

theneva force-pushed the unused-mock-interceptor branch from 11ff74f to 6af598d Compare April 25, 2022 14:16

SimenB approved these changes Apr 25, 2022

View reviewed changes

theneva commented Apr 25, 2022

View reviewed changes

mcollina approved these changes Apr 25, 2022

View reviewed changes

mcollina merged commit 2e5171b into nodejs:main Apr 29, 2022

theneva deleted the unused-mock-interceptor branch April 29, 2022 20:36

KhafraDev pushed a commit to KhafraDev/undici that referenced this pull request Jun 23, 2022

feat: add pending interceptor check functions to mock agent (nodejs#1358

2ae0dcc

)

metcoder95 pushed a commit to metcoder95/undici that referenced this pull request Dec 26, 2022

feat: add pending interceptor check functions to mock agent (nodejs#1358

1f773fb

)

crysmags pushed a commit to crysmags/undici that referenced this pull request Feb 27, 2024

feat: add pending interceptor check functions to mock agent (nodejs#1358

01f65bc

)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add pending interceptor check functions to mock agent #1358

feat: add pending interceptor check functions to mock agent #1358

theneva commented Apr 21, 2022 •

edited

mcollina left a comment

theneva commented Apr 22, 2022

mcollina commented Apr 22, 2022

theneva commented Apr 22, 2022

codecov-commenter commented Apr 22, 2022 •

edited

theneva commented Apr 22, 2022

mcollina commented Apr 23, 2022

theneva commented Apr 23, 2022 •

edited

mcollina left a comment

SimenB left a comment

SimenB Apr 24, 2022

theneva Apr 25, 2022

mcollina Apr 25, 2022

theneva Apr 25, 2022

SimenB left a comment

theneva commented Apr 25, 2022 •

edited

theneva Apr 25, 2022

theneva Apr 25, 2022

theneva Apr 25, 2022

theneva Apr 25, 2022

theneva Apr 25, 2022

theneva Apr 25, 2022

theneva Apr 25, 2022

mcollina left a comment

SimenB commented Apr 25, 2022

theneva commented Apr 29, 2022

SimenB commented Apr 30, 2022

SimenB commented May 2, 2022 •

edited

feat: add pending interceptor check functions to mock agent #1358

feat: add pending interceptor check functions to mock agent #1358

Conversation

theneva commented Apr 21, 2022 • edited

mcollina left a comment

Choose a reason for hiding this comment

theneva commented Apr 22, 2022

mcollina commented Apr 22, 2022

theneva commented Apr 22, 2022

codecov-commenter commented Apr 22, 2022 • edited

Codecov Report

theneva commented Apr 22, 2022

mcollina commented Apr 23, 2022

theneva commented Apr 23, 2022 • edited

mcollina left a comment

Choose a reason for hiding this comment

SimenB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimenB left a comment

Choose a reason for hiding this comment

theneva commented Apr 25, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

SimenB commented Apr 25, 2022

theneva commented Apr 29, 2022

SimenB commented Apr 30, 2022

SimenB commented May 2, 2022 • edited

theneva commented Apr 21, 2022 •

edited

codecov-commenter commented Apr 22, 2022 •

edited

theneva commented Apr 23, 2022 •

edited

theneva commented Apr 25, 2022 •

edited

SimenB commented May 2, 2022 •

edited