Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mrc 6022 - vitest server pt 2 #235

Open
wants to merge 2 commits into
base: mrc-6021
Choose a base branch
from
Open

Mrc 6022 - vitest server pt 2 #235

wants to merge 2 commits into from

Conversation

M-Kusumgar
Copy link
Collaborator

@M-Kusumgar M-Kusumgar commented Nov 16, 2024

This finishes the tests, points of interest have been highlighted by comments, the only other thing ive done is update wodin versions, we were on versions 0.1.0 and 0.3.0 for frontend and backend respectively (in main), after this upgrade i just went up major versions to 1.0.0 for both, seems alright to me after this big upgrade!

also genversion is a package we use to generate version thats in the package.json into the version.ts file, not sure why we need a package for this if im being honest but i just put it in for now cos its a very slim package

Copy link

codecov bot commented Nov 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.61%. Comparing base (748c80f) to head (cff174f).

Additional details and impacted files
@@             Coverage Diff              @@
##           mrc-6021     #235      +/-   ##
============================================
- Coverage     99.78%   99.61%   -0.17%     
============================================
  Files           159      183      +24     
  Lines          4107     4462     +355     
  Branches        936      987      +51     
============================================
+ Hits           4098     4445     +347     
- Misses            8       15       +7     
- Partials          1        2       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -50,13 +50,13 @@ describe("apiService", () => {
headers: AxiosRequestHeaders,
transformResponse: AxiosResponseTransformer
) => {
expect(headers).toStrictEqual({
expect(headers.toJSON()).toEqual({
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

axios code changed as we updated the package, so just have to explicitly convert to json before checking

Comment on lines +2 to +3
const { mockJsonResponseError } = vi.hoisted(() => ({ mockJsonResponseError: vi.fn() }));
vi.mock("../src/jsonResponse", () => { return { jsonResponseError: mockJsonResponseError }; });
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mockJsonResponseError has to be hoisted as it is used in the vi.mock and in the tests

expect(errorCallback).not.toHaveBeenCalled();
done();
}, 200);
await new Promise(res => setTimeout(res, 200));
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of setTimeout and using done callback (which is now completely deprecated) we just await 200ms before moving on

Comment on lines +3 to +18
import bodyParser from "body-parser";

describe("odin routes", () => {
const express = require("express");
const bodyParser = require("body-parser");
const { mockRouter } = vi.hoisted(() => ({
mockRouter: {
get: vi.fn(),
post: vi.fn()
}
}));

const mockRouter = {
get: jest.fn(),
post: jest.fn()
};
const realRouter = express.Router;
vi.mock("express", () => ({
Router: () => mockRouter
}));

const spyText = jest.spyOn(bodyParser, "text");

beforeAll(() => {
express.Router = () => mockRouter;
describe("odin routes", async () => {
beforeEach(() => {
vi.resetAllMocks()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was weird, we were importing and then reassigning an imported thing as something else and then resetting it with the real router? this is just a roundabout of just mocking the package so thats what i changed this to, now im just mocking the router

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have done this same thing in a couple other places below

@@ -30,7 +30,7 @@ describe("registerRoutes", () => {
} as any;

it("registers all expected routes", () => {
const router = registerRoutes(mockApp);
const router = registerRoutes(mockApp) as unknown as typeof mockRouter;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

forcing type because we know it will return the mockRouter in this test so just helping typescript along

Copy link
Collaborator

@absternator absternator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants