[api] migrate testing framework (Jest > Vitest) #3555

freemvmt · 2024-08-24T17:19:01Z

This PR is motivated by Jest's failure to handle ESM modules sufficiently well, which was a blocker on the Microsoft SSO work (ticket). In the end we decided to migrate away from Jest, with Vitest being the obvious alternative, because a quick experiment with it showed promise (see this comment).

Ticket: https://trello.com/c/N1egz8at

So, this PR does the following:

Add vitest package and configure (e.g. vitest.config.ts, setup files, package.json scripts etc)
Adapts all tests in api.planx.uk to use vitest instead of jest - this was the most laborious step and I will document some of the thornier aspects in my own review shortly (Vitest's dedicated migration guide was very useful)
Ensures coverage works similarly (e.g. relies on istanbul, keeps existing thresholds, displays in browser etc)
Removes all things Jest

A note on coverage

The text-summary coverage results are slightly different in this branch compared with main.

main (jest)

Statements   : 78.74% ( 2489/3161 )
Branches     : 54.96% ( 299/544 )
Functions    : 67.67% ( 312/461 )
Lines        : 77.95% ( 2305/2957 )

api-migrate-jest-vitest (vitest)

Statements   : 71.94% ( 1726/2399 )
Branches     : 54.92% ( 413/752 )
Functions    : 67.47% ( 305/452 )
Lines        : 71.75% ( 1659/2312 )

I haven't seriously investigated the cause of this discrepancy because it doesn't seem a substantial change. My instinct is that it's due to the default exclusions of each framework - e.g. as far as I can see, vitest has a long list of exclusions, whereas jest just ignores node_modules (or perhaps also excludes the istanbul defaults). But it might be some other implementation detail.

If we wanted to try and create a culture of always-increasing coverage, we could adopt the autoUpdate flag, as per my comment in vitest.config.ts. But for now I've stuck with the singular minimum coverage threshold of 55% of functions.

freemvmt

Some comments for easier digestion!

freemvmt · 2024-08-24T17:22:14Z

api.planx.uk/lib/hasura/metadata/index.test.ts

-  post: jest.fn(),
-}));
-const mockAxios = Axios as jest.Mocked<typeof Axios>;
+describe("Creation of scheduled event", () => {


Wrapped tests which I adapted in a describe if it didn't already exist 🫡

freemvmt · 2024-08-24T17:24:15Z

api.planx.uk/lib/hasura/metadata/index.test.ts

-}));
-const mockAxios = Axios as jest.Mocked<typeof Axios>;
+describe("Creation of scheduled event", () => {
+  vi.mock("axios", async (importOriginal) => {


vitest has this importOriginal argument which is passed to the factory constructor in a mock call - we can use this to import the actual module (replacing jest.requireActual), as per the docs.

Could also have used vi.importActual here, but using the arg seemed neater.

freemvmt · 2024-08-24T17:24:54Z

api.planx.uk/lib/hasura/metadata/index.test.ts

+  vi.mock("axios", async (importOriginal) => {
+    const actualAxios = await importOriginal<typeof import("axios")>();
+    return {
+      default: {


Vitest does not assume, like jest, that a returned object is the default export of given module, so we have to be explicit about it, as per the docs.

freemvmt · 2024-08-24T17:26:33Z

api.planx.uk/modules/admin/session/csv.test.ts

@@ -23,7 +23,9 @@ jest.mock("@opensystemslab/planx-core", () => {
 });

 describe("CSV data admin endpoint", () => {
-  afterEach(() => jest.clearAllMocks());
+  afterEach(() => {


hooks like afterEach have to return null or undefined, so have remove implicit return, as per docs

freemvmt · 2024-08-24T17:27:24Z

api.planx.uk/modules/gis/service/classifiedRoads.test.ts

@@ -18,7 +18,7 @@ it("returns an error if required query param is missing", async () => {
 // "Success" test commented out due to reliance on external API calls and fallibility of nocks
 //   Please comment in and run locally if making changes to /roads functionality
 describe.skip("fetching classified roads data from OS Features API for any local authority", () => {
-  jest.setTimeout(10000);
+  vi.setConfig({ testTimeout: 1000 });


See relevant docs

freemvmt · 2024-08-24T17:28:01Z

api.planx.uk/modules/gis/service/helpers.test.js

@@ -151,7 +151,6 @@ describe("rollupResultLayer helper function", () => {
    // Assert
    expect(result).toHaveProperty(key);
    layersToRollup.forEach((layer) => {
-      // Jest can handle paths using dot notation, so keys with a dot need to be wrapped in []


I assume this was supposed to say 'cannot handle...' ?

I actually think "can" was correct here - the key here would have been something like a.b.c which Jest would have tried to default to a nested path, as opposed to the object in question which is likely { "a.b.c": "x" }

Either way, it's a good call to remove this comment - it looks like vitest handles this more gracefully - the test actually passes with or without the wrapping [] on line 154.

freemvmt · 2024-08-24T17:28:40Z

api.planx.uk/modules/pay/service/inviteToPay/createPaymentSendEvents.test.ts

@@ -3,9 +3,10 @@ import app from "../../../../server.js";
 import { createScheduledEvent } from "../../../../lib/hasura/metadata/index.js";
 import { queryMock } from "../../../../tests/graphqlQueryMock.js";
 import { flowWithInviteToPay } from "../../../../tests/mocks/inviteToPayData.js";
+import { MockedFunction } from "vitest";


no vi namespace so we have to explicitly import types like this one, as per docs

freemvmt · 2024-08-24T17:31:03Z

api.planx.uk/modules/slack/index.test.ts

@@ -12,14 +12,16 @@ const invalidBody = {
  wrong: "message",
 };

-const mockSend = jest.fn();
-jest.mock<typeof SlackNotify>("slack-notify", () =>


tsc/vitest seem smart enough not to need the type declaration anymore 🤔

Ah that's awesome because this step always felt awkward and brittle in jest imho 🙌

freemvmt · 2024-08-24T17:33:23Z

api.planx.uk/package.json

-    "test:watch": "TZ=Europe/London NODE_OPTIONS='$NODE_OPTIONS --experimental-vm-modules' jest --coverage=false --watch",
+    "test": "TZ=Europe/London vitest run",
+    "test:coverage": "TZ=Europe/London vitest run --coverage && open coverage/lcov-report/index.html",
+    "test:watch": "TZ=Europe/London vitest --ui --coverage",


watch mode now throws up a nice UI like below

github-actions · 2024-08-24T17:41:35Z

Removed vultr server and associated DNS entries

DafyddLlyr

Fantastic PR - thanks so much for the documentation links and explanations, super helpful.

DafyddLlyr · 2024-08-26T08:23:29Z

api.planx.uk/.eslintrc

  "extends": [
    "eslint:recommended",
    "plugin:@typescript-eslint/recommended",
-    "prettier",
-    "plugin:jest/recommended"


Super happy for this to be a follow up PR - it might be worth looking at something like https://www.npmjs.com/package/eslint-plugin-vitest to replace this?

DafyddLlyr · 2024-08-26T08:31:07Z

api.planx.uk/modules/gis/service/helpers.test.js

@@ -151,7 +151,6 @@ describe("rollupResultLayer helper function", () => {
    // Assert
    expect(result).toHaveProperty(key);
    layersToRollup.forEach((layer) => {
-      // Jest can handle paths using dot notation, so keys with a dot need to be wrapped in []


I actually think "can" was correct here - the key here would have been something like a.b.c which Jest would have tried to default to a nested path, as opposed to the object in question which is likely { "a.b.c": "x" }

Either way, it's a good call to remove this comment - it looks like vitest handles this more gracefully - the test actually passes with or without the wrapping [] on line 154.

DafyddLlyr · 2024-08-26T08:33:32Z

api.planx.uk/vitest.config.ts

Really appreciate the comments in this config file 💪

DafyddLlyr · 2024-08-26T08:35:14Z

api.planx.uk/vitest.config.ts

+        functions: 55,
+        // TODO: could add autoUpdate flag here so that function coverage is only allowed to increase


Let's do this and leave this better than we found it 👍

…to Vitest

DafyddLlyr · 2024-08-29T07:22:20Z

Merged in @freemvmt 's absence.

I'll pick up the two (very small) tasks suggested above in the comments -

Use the autoUpdate flag for API tests
- test(api): Add autoUpdate flag to API Vitest config #3586
Replace ESLint "plugin:jest/recommended" with a vitest equivalent
- test(api): Setup eslint-plugin-vite #3587

freemvmt requested a review from a team August 24, 2024 17:19

freemvmt commented Aug 24, 2024

View reviewed changes

freemvmt mentioned this pull request Aug 24, 2024

[api] add Microsoft strategy to auth module (single sign-on) #3453

Merged

16 tasks

DafyddLlyr approved these changes Aug 26, 2024

View reviewed changes

freemvmt added 5 commits August 29, 2024 08:06

[api] add config, package.json scripts and setup logic for migration …

a741b75

…to Vitest

[api] adapt tests to succeed with vitest

b05151b

[api] mirror jest coverage setup for vitest

7b7b41c

[api] clean up all traces of jest, remove associated packages

5702e3f

[api] fix minor typo

c870585

DafyddLlyr force-pushed the api-migrate-jest-vitest branch from a94d651 to c870585 Compare August 29, 2024 07:06

DafyddLlyr merged commit 3984823 into main Aug 29, 2024
12 checks passed

DafyddLlyr deleted the api-migrate-jest-vitest branch August 29, 2024 07:22

This was referenced Aug 29, 2024

test(api): Add autoUpdate flag to API Vitest config #3586

Merged

test(api): Setup eslint-plugin-vite #3587

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[api] migrate testing framework (Jest > Vitest) #3555

[api] migrate testing framework (Jest > Vitest) #3555

freemvmt commented Aug 24, 2024 •

edited

Loading

freemvmt left a comment

freemvmt Aug 24, 2024

freemvmt Aug 24, 2024

freemvmt Aug 24, 2024

freemvmt Aug 24, 2024

freemvmt Aug 24, 2024

freemvmt Aug 24, 2024

DafyddLlyr Aug 26, 2024

freemvmt Aug 24, 2024

freemvmt Aug 24, 2024

DafyddLlyr Aug 26, 2024

freemvmt Aug 24, 2024

github-actions bot commented Aug 24, 2024 •

edited

Loading

DafyddLlyr left a comment

DafyddLlyr Aug 26, 2024

DafyddLlyr Aug 26, 2024

DafyddLlyr Aug 26, 2024

DafyddLlyr Aug 26, 2024

DafyddLlyr commented Aug 29, 2024 •

edited

Loading

		functions: 55,
		// TODO: could add autoUpdate flag here so that function coverage is only allowed to increase

[api] migrate testing framework (Jest > Vitest) #3555

[api] migrate testing framework (Jest > Vitest) #3555

Conversation

freemvmt commented Aug 24, 2024 • edited Loading

A note on coverage

main (jest)

api-migrate-jest-vitest (vitest)

freemvmt 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

github-actions bot commented Aug 24, 2024 • edited Loading

DafyddLlyr 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

DafyddLlyr commented Aug 29, 2024 • edited Loading

freemvmt commented Aug 24, 2024 •

edited

Loading

github-actions bot commented Aug 24, 2024 •

edited

Loading

DafyddLlyr commented Aug 29, 2024 •

edited

Loading