Avoid JSON.stringify hack when testing responses of fetch-mock

[oscard0m]

Affected repos:

• feat: v20 rest.js#305
• feat: v5 #563
• fix(deps): update octokit monorepo (major) action.js#513
• build(deps): lock file maintenance auth-app.js#498
• feat: v5 oauth-app.js#343

---

• Replace toStrictEqual with JSON.stringify Response assertions with toEqual in core.js #590
• Replace toStrictEqual with JSON.stringify Response assertions with toEqual in rest.js rest.js#328
• Replace toStrictEqual with JSON.stringify Response assertions with toEqual in action.js action.js#515
• Replace toMatchObject Response assertions with toEqual in auth-app.js auth-app.js#506
• Replace toMatchObject Response assertions with toEqual in oauth-app.js oauth-app.js#437

---

          this should not be necessary, but if it's tricky we can do a follow up issue. It slows down tests, makes order of keys significant, and will be harder to debug when they don't match.

Originally posted by @gr2m in #563 (comment)

[oscard0m]

I took a look into it.

The toStrictEqual is doing some customTesters internally. The one giving problems is typeEquality. Concretely, this comparison: a.constructor === b.constructor.

When using fetch-mock, the accepted type for response parameter in getOnce(matcher, response) are:

type MockResponse = Response | Promise<Response>
                        | number | Promise<number>
                        | string | Promise<string>
                        | {} | Promise<{}>
                        | MockResponseObject | Promise<MockResponseObject>;

According to this code comment:

• object: As long as the object is not a MockResponseObject it is
  * converted into a json string and returned as the body of a 200 response

This means the constructor equal comparison is lost at this point.¹²

Considering these are mocks controlled by ourselves, the alternatives I come up with are:
a) to use a jest.toEqual
b) to keep the hack of JSON.stringify
c) try if passing a MockResponseObject or Response gets the right constructor and toStrictEqual works.

What do you think @octokit/extensibility-sdks @octokit/js-community ?

Footnotes

1. https://github.com/gr2m/fetch-mock/blob/1bdac54e1b482398cca0af9658a500de23e72367/src/lib/fetch-handler.js#L268-L278 ↩

2. https://github.com/gr2m/fetch-mock/blob/1bdac54e1b482398cca0af9658a500de23e72367/src/lib/response-builder.js#L113 ↩

[kfcampbell]

I think I have a weak preference in favor of jest.ToEqual. I had to remind myself of the differences when looking at .toStrictEqual docs:

• keys with undefined properties are checked, e.g. {a: undefined, b: 2} will not equal {b: 2};
• undefined items are taken into account, e.g. [2] will not equal [2, undefined];
• array sparseness is checked, e.g. [, 1] will not equal [undefined, 1];
• object types are checked, e.g. a class instance with fields a and b will not equal a literal object with fields a and b.

None of those seem deal-breaking to me, though I'd be interested to hear others' thoughts.

[oscard0m]

Thumbs up reaction (👍🏽) for jest.ToEqual, and I can apply the changes here:

• feat: v20 rest.js#305
• feat: v5 #563
• fix(deps): update octokit monorepo (major) action.js#513

[gr2m]

Thumbs up reaction (👍🏽) for jest.ToEqual, and I can apply the changes here:

👍🏼 from me, thank you @oscard0m for taking on this and other chore tasks 💛

[wolfy1339]

There is at least one case where I switched to toMatchObject()

Those could probably be updated as well

[oscard0m]

There is at least one case where I switched to toMatchObject()

Those could probably be updated as well

In a quick search these are the ones I found:

• octokit/auth-app.js@d5fe818
• octokit/oauth-app.js@b672135

The rest can be progressively updated. It's not a problem to have jest.toMatchObject().

[oscard0m]

Thank you all for the quick reviews. We are done with this. 🥳 🎆 🍾