Skip to content

Commit

Permalink
add a test for get server side props
Browse files Browse the repository at this point in the history
  • Loading branch information
@aligg
aligg committed Dec 8, 2023
1 parent e038ff4 commit 62b1f5b
Showing 1 changed file with 16 additions and 1 deletion.
17 changes: 16 additions & 1 deletion app/tests/pages/index.test.tsx
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import { axe } from "jest-axe";
import Index from "src/pages/index";
import { GetServerSidePropsContext } from "next";
import Index, { getServerSideProps } from "src/pages/index";
import { LocalFeatureFlagManager } from "src/services/feature-flags/LocalFeatureFlagManager";
import { render, screen } from "tests/react-utils";

describe("Index", () => {
Expand All @@ -25,4 +27,17 @@ describe("Index", () => {
const { container } = render(<Index isFooEnabled={true} />);
expect(container).toHaveTextContent("Flag is enabled");
});

it("retrieves feature flags", async () => {

This comment has been minimized.

Sign in to view

Copy link
@lorenyu

lorenyu Dec 8, 2023

Contributor

@aligg couple of thoughts on this test:

  1. Can we avoid spies and mocks for this test? I don’t think they are necessary. It will make the test easier to read and maintain
  2. I’m not familiar with Nextjs testing patterns but needing to get the server context and call getServerProps directly seems too coupled to implementation details of Nextjs and makes the test harder to understand and maintain. Can we instead use standard js interfaces, like can make an http request

This comment has been minimized.

Sign in to view

Copy link
@sawyerh

sawyerh Dec 8, 2023

Contributor

I think point 2 is moot in an App Router architecture since there's no getServerSideProps and the data fetching is just part of the component, so in an App Router world, it will be executed when the component is rendered. I don't think it would be possible to send an HTTP request to the page from a test since that would be dependent on the rest of Next.js server running?

This comment has been minimized.

Sign in to view

Copy link
@lorenyu

lorenyu Dec 8, 2023

Contributor

I think point 2 is moot in an App Router architecture since there's no getServerSideProps and the data fetching is just part of the component, so in an App Router world, it will be executed when the component is rendered.

:) actually the fact that getServerSideProps is obsolete in an App Router is precisely the reason why the test should not be coupled to implementation details of Nextjs (i.e. the test shouldn't care whether Nextjs uses App Router or Page router under the hood).

I don't think it would be possible to send an HTTP request to the page from a test since that would be dependent on the rest of Next.js server running?

If it's not possible to create an "instance of the Next.js server" that we can send requests to (which would be a bummer in my opinion since you could do that for expressjs or flask or other frameworks i'm familiar with), then i think the only option is to actually spin up the server locally and send real network requests to it.

This comment has been minimized.

Sign in to view

Copy link
@aligg

aligg Dec 9, 2023

Author Contributor

Filed a ticket for this: #265 @lorenyu if you have clear vision on what you'd like to see instead, feel free to update the ticket (I wasn't sure what you were envisioning besides a spy w/r/t point 1 for example). Additionally I didn't include any follow up w/r/t point 2 yet.

This comment has been minimized.

Sign in to view

Copy link
@lorenyu

lorenyu Dec 11, 2023

Contributor

I'm sorry. Thinking about it more, I realize that having e2e tests for these endpoints is probably overkill. If we have a good practice of minimizing logic in page endpoints and instead put all business logic in API endpoints then we shouldn't need to worry about having server-side tests for the pages.

I still have same concerns about how to test API endpoints for Next.js. I can't seem to find any good docs on it that don't involve just mocking out everything. Next.js seems to make a lot of common best practices really hard to do.

This comment has been minimized.

Sign in to view

Copy link
@sawyerh

sawyerh Dec 11, 2023

Contributor

I still have same concerns about how to test API endpoints for Next.js. I can't seem to find any good docs on it that don't involve just mocking out everything.

I may be misunderstanding but API endpoints are functions that receive a Request so they should be able to be tested similarly to any other function.

This comment has been minimized.

Sign in to view

Copy link
@lorenyu

lorenyu Dec 11, 2023

Contributor

Ah I see, nice, yes I think that would address my concern. Thanks for the education!
const featureName = "foo";
const userId = "anonymous";
const featureFlagSpy = jest
.spyOn(LocalFeatureFlagManager.prototype, "isFeatureEnabled")
.mockResolvedValue(true);
await getServerSideProps({
req: { cookies: {} },
res: {},
} as unknown as GetServerSidePropsContext);
expect(featureFlagSpy).toHaveBeenCalledWith(featureName, userId);
});
});

0 comments on commit 62b1f5b

Please sign in to comment.