feat: Migrate to undici MockClient #79

oliversalzburg · 2023-10-27T16:10:00Z

Breaking change!

Internally uses undici's MockAgent, instead of nock to mock requests.
nock is still used for data-matching to retain backwards compatibility.
Tests now use fetch instead of axios.

Requires node>=18 and @octokit/rest>=20

Closes #77

Breaking change! - Internally uses `undici`'s `MockAgent`, instead of `nock` to mock requests. - `nock` is still used for data-matching to retain backwards compatibility. - Tests now use `fetch` instead of `axios`. Requires node>=18 and @octokit/rest>=20 Closes kiegroup#77

shubhbapna · 2023-10-30T02:13:36Z

Thank you for your PR! I will take a look at the changes over this week

oliversalzburg · 2023-11-13T09:17:58Z

@shubhbapna Please let me know if the chance to have this merged is low. I can also transition my project to my fork.

shubhbapna

So sorry for the delay! It slipped through the cracks for me.

shubhbapna · 2023-11-14T15:46:31Z

.github/workflows/publish.yml

@@ -14,7 +14,7 @@ jobs:
      - uses: actions/checkout@v3
      - uses: actions/setup-node@v3
        with:
-          node-version: 16
+          node-version: 20


I would like to support node 18 as well, so probably change it to matrix and test for both? Same for all the other workflow files

Sounds reasonable.

Wait, this is the publish job. The matrix is already in pull_request 😄 I'll add 18 there.

src/moctokit/moctokit.ts

shubhbapna · 2023-11-14T15:55:18Z

test/moctokit/moctokit.test.ts

@@ -15,14 +15,14 @@ test("with default base url", async () => {
    repo: "project",
  });
  expect(data1.status).toBe(200);
-  expect(data1.data).toStrictEqual({ full_name: "it definitely worked" });
+  expect(data1.data).toEqual({ full_name: "it definitely worked" });


why was this change needed?

I guess it wasn't. It must have been an oversight when rewriting the tests.

shubhbapna · 2023-11-14T15:58:23Z

scripts/endpoint-requests.js

@@ -42,21 +42,23 @@ async function generateEndpointRequest() {

    newEndpointRequest[scope][
      endpoint.id
-    ] = `new MoctokitRequestMocker<"${path}", "${method}">(baseUrl, endpoints["${scope}"]["${endpoint.id}"], allowUnmocked).request`;


why was allowUnmocked removed?

This behavior is controlled globally through https://undici.nodejs.org/#/docs/api/MockAgent?id=mockagentdisablenetconnect in undici.

Could we keep such that the user can decide the behaviour? So if the user passes allowUnmocked as false then we call MockAgent.disableNetConnect() otherwise we do nothing

I'm doing that in EndpointMocker actually 😊

shubhbapna · 2023-11-14T16:06:55Z

src/endpoint-mocker/abstract-endpoint-mocker.ts

+    if (process.env.JEST_WORKER_ID !== undefined) {
+      // @ts-expect-error This seems impossible to type correctly.
+      globalThis.fetch = (
+        input: NodeJS.fetch.RequestInfo,
+        init?: NodeJS.fetch.RequestInit | undefined
+      ) =>
+        fetch(input, {
+          dispatcher: this.agent,
+          ...(init as undiciRequestInit),
+        });
+    }
+  }


Could you explain why we need this? I think this is more of a jest issue where the jest globals differ from node globals: jestjs/jest#2549. So we could leave the workaround for this user where, the user has to configure the globals correctly

It could probably be moved to the test suites themselves. Of course, all consumers of this package will have to do that too for all their test suites, if they use Jest. Could be painful. Or maybe I'm missing a better solution.

I see, lets keep it for now

shubhbapna · 2023-11-14T16:37:37Z

src/endpoint-mocker/response/abstract-response-mocker.ts

+        path: requestPath => this.pathHandler(requestPath),
+        method: this.method.toUpperCase(),
+        body: requestBody => this.bodyHandler(requestBody),
+        query: this.query,


I wasn't able to find the docs on this, but does undici match queries based on regex as well?

Queries are matched as part of the path. See further comments on this below.

src/endpoint-mocker/response/abstract-response-mocker.ts

shubhbapna · 2023-11-14T16:45:28Z

test/moctokit/request-response-mocker.test.ts

@@ -1,18 +1,18 @@
 /* eslint-disable @typescript-eslint/no-explicit-any */
-import axios from "axios";


Why was axios removed? I wouldn't change the existing test cases

axios doesn't use Node's fetch implementation and isn't affected by undici mocks.

hmm this PR would break backward compatibility altogether in the case. I think a better solution would be to have nock and as well as undici mocks. What do you think?

That would definitely be an option.

I've seen several other projects introduce a similar breaking change when they moved to Node18 and beyond. Many projects only support Node18 and newer now, at least in my part of the bubble :)

However, as I remember, the new Octokit tooling also has a new fetch-based, and the old interface still built in.

This also made me think about if both paths should be implemented in mock-github. I felt like users of old octokit versions could still use the existing version of mock-github; the path into the future will be entirely fetch-based, as that is what the latest implementations from GitHub use. If you are mocking Octokit, it is likely that you want to work with the latest supported version, which uses fetch by default in the latest version, and is highly likely to continue doing so moving ahead.

Given all that, I felt like it could make sense to just accept the breaking change and focus on a single implementation.

If you think it's worth the effort to maintain the two paths, I'm happy to work on that as well :)

oliversalzburg

I addressed the comments and I'll work on some changes.

oliversalzburg · 2023-11-14T17:02:50Z

test/moctokit/moctokit.test.ts

@@ -15,14 +15,14 @@ test("with default base url", async () => {
    repo: "project",
  });
  expect(data1.status).toBe(200);
-  expect(data1.data).toStrictEqual({ full_name: "it definitely worked" });
+  expect(data1.data).toEqual({ full_name: "it definitely worked" });


I guess it wasn't. It must have been an oversight when rewriting the tests.

oliversalzburg · 2023-11-14T17:05:43Z

test/moctokit/request-response-mocker.test.ts

@@ -1,18 +1,18 @@
 /* eslint-disable @typescript-eslint/no-explicit-any */
-import axios from "axios";


axios doesn't use Node's fetch implementation and isn't affected by undici mocks.

src/endpoint-mocker/response/abstract-response-mocker.ts

oliversalzburg · 2023-11-14T18:00:35Z

src/endpoint-mocker/response/abstract-response-mocker.ts

+        path: requestPath => this.pathHandler(requestPath),
+        method: this.method.toUpperCase(),
+        body: requestBody => this.bodyHandler(requestBody),
+        query: this.query,


Queries are matched as part of the path. See further comments on this below.

src/endpoint-mocker/response/abstract-response-mocker.ts

oliversalzburg · 2023-11-14T18:08:39Z

src/endpoint-mocker/abstract-endpoint-mocker.ts

+    if (process.env.JEST_WORKER_ID !== undefined) {
+      // @ts-expect-error This seems impossible to type correctly.
+      globalThis.fetch = (
+        input: NodeJS.fetch.RequestInfo,
+        init?: NodeJS.fetch.RequestInit | undefined
+      ) =>
+        fetch(input, {
+          dispatcher: this.agent,
+          ...(init as undiciRequestInit),
+        });
+    }
+  }


It could probably be moved to the test suites themselves. Of course, all consumers of this package will have to do that too for all their test suites, if they use Jest. Could be painful. Or maybe I'm missing a better solution.

oliversalzburg · 2023-11-14T18:10:04Z

scripts/endpoint-requests.js

@@ -42,21 +42,23 @@ async function generateEndpointRequest() {

    newEndpointRequest[scope][
      endpoint.id
-    ] = `new MoctokitRequestMocker<"${path}", "${method}">(baseUrl, endpoints["${scope}"]["${endpoint.id}"], allowUnmocked).request`;


This behavior is controlled globally through https://undici.nodejs.org/#/docs/api/MockAgent?id=mockagentdisablenetconnect in undici.

oliversalzburg

I hope I addressed everything :)

oliversalzburg · 2023-11-22T15:02:52Z

.github/workflows/publish.yml

@@ -14,7 +14,7 @@ jobs:
      - uses: actions/checkout@v3
      - uses: actions/setup-node@v3
        with:
-          node-version: 16
+          node-version: 20


Wait, this is the publish job. The matrix is already in pull_request 😄 I'll add 18 there.

oliversalzburg · 2023-11-22T15:04:34Z

scripts/endpoint-requests.js

@@ -42,21 +42,23 @@ async function generateEndpointRequest() {

    newEndpointRequest[scope][
      endpoint.id
-    ] = `new MoctokitRequestMocker<"${path}", "${method}">(baseUrl, endpoints["${scope}"]["${endpoint.id}"], allowUnmocked).request`;


I'm doing that in EndpointMocker actually 😊

oliversalzburg · 2023-11-22T15:06:49Z

src/endpoint-mocker/response/abstract-response-mocker.ts

+        .reply(response.status, response.data as Record<string, unknown>, {
+          headers: {
+            ...(response.headers as IncomingHttpHeaders),
+            "content-type": this.getContentTypeForResponseData(response.data),


I don't follow. In the headers hash, the content-type is the first entry, then we spread the user-defined response.headers (which could also include a content-type) into the hash, overriding any existing properties.

Shouldn't that already have the desired effect?

shubhbapna · 2023-12-27T07:26:07Z

Hi, sorry I was away for sometime. I was recently made aware of the ongoing effort in nock to support undici: nock/nock#2517

They estimate that this work will be done by January. I would rather wait for this and have the mocking entirely handled by nock which in the longer run will be better and will also reduce the maintenance load on this project. So for now I will mark this pull request as draft. Sorry for the inconvenience and thank you so much for the contribution!

oliversalzburg force-pushed the feat/node18-fetch branch from a196cef to 61ea9ff Compare October 27, 2023 16:17

oliversalzburg force-pushed the feat/node18-fetch branch from 61ea9ff to 74fa0bc Compare October 27, 2023 16:38

shubhbapna self-requested a review October 30, 2023 19:55

shubhbapna requested changes Nov 14, 2023

View reviewed changes

oliversalzburg added 5 commits November 14, 2023 17:53

Fix unused import

e94e037

Use strict equal comparison

99191d7

Allow user to override content-type

02fd08a

Fix missing header matching

1a6df34

Use @mg import

1b4d0a9

oliversalzburg commented Nov 14, 2023

View reviewed changes

Also test on Node18

f489538

oliversalzburg commented Nov 22, 2023

View reviewed changes

shubhbapna marked this pull request as draft December 27, 2023 07:26

oliversalzburg closed this Dec 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Migrate to undici MockClient #79

feat: Migrate to undici MockClient #79

oliversalzburg commented Oct 27, 2023 •

edited

shubhbapna commented Oct 30, 2023

oliversalzburg commented Nov 13, 2023

shubhbapna left a comment

shubhbapna Nov 14, 2023

oliversalzburg Nov 14, 2023

oliversalzburg Nov 22, 2023

shubhbapna Nov 14, 2023

oliversalzburg Nov 14, 2023

shubhbapna Nov 14, 2023

oliversalzburg Nov 14, 2023

shubhbapna Nov 21, 2023

oliversalzburg Nov 22, 2023

shubhbapna Nov 14, 2023

oliversalzburg Nov 14, 2023

shubhbapna Nov 21, 2023

shubhbapna Nov 14, 2023

oliversalzburg Nov 14, 2023

shubhbapna Nov 14, 2023

oliversalzburg Nov 14, 2023

shubhbapna Nov 21, 2023

oliversalzburg Nov 22, 2023

oliversalzburg left a comment

oliversalzburg Nov 14, 2023

oliversalzburg Nov 14, 2023

oliversalzburg Nov 14, 2023

oliversalzburg Nov 14, 2023

oliversalzburg Nov 14, 2023

oliversalzburg left a comment

oliversalzburg Nov 22, 2023

oliversalzburg Nov 22, 2023

oliversalzburg Nov 22, 2023

shubhbapna commented Dec 27, 2023

		@@ -1,18 +1,18 @@
		/* eslint-disable @typescript-eslint/no-explicit-any */
		import axios from "axios";

feat: Migrate to undici MockClient #79

feat: Migrate to undici MockClient #79

Conversation

oliversalzburg commented Oct 27, 2023 • edited

shubhbapna commented Oct 30, 2023

oliversalzburg commented Nov 13, 2023

shubhbapna 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

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

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

oliversalzburg 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

Choose a reason for hiding this comment

oliversalzburg 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

shubhbapna commented Dec 27, 2023

oliversalzburg commented Oct 27, 2023 •

edited