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

Any guidance on testing components that utilize react-oidc? #2

Closed
tostringtheory opened this issue Jun 30, 2020 · 8 comments
Closed

Comments

@tostringtheory
Copy link

@connelhooley - thanks for providing a really useful package that immediately filled some of my needs in my project. Very elegant/clean solution.

My main/major question thus far however, is do you have any guidance on how best to mock/test a component, when there's some dependencies on the react-oidc library? I am using jest with @testing-library/react.

Take for example a component that depends on Authorized, NotAuthorized, and Authorizing. All 3 of these depend on useUser, which then depends on AuthProvider. I'll be honest, my jest mocking skills aren't the greatest, and I can't figure out for the life of me how to best mock being in the context of a user.

The closest I came was to mock the entire library via a jest.mock call, and return a module mock:

jest.mock("@connelhooley/react-oidc", () => ({
	...(jest.requireActual("@connelhooley/react-oidc") as Record<string, unknown>),
	useUser: jest.fn(),
	SignIn: () => (<div>Redirect to Sign In</div>),
	Authorized: ({ children }: { children: React.ReactNode }) => (<>{children}</>),
	Authorizing: ({ children }: { children: React.ReactNode }) => (<>{children}</>),
	NotAuthorized: ({ children }: { children: React.ReactNode }) => (<>{children}</>)
}));

This really however seems like a hack and I feel there's got to be a better way. I figured you might have some idea seeing as how I see you have some tests in this project (I tried to utilize the same strategies to no avail), but figured you may be using this library in something else of yours with some tests too.

@tostringtheory
Copy link
Author

Well, I was finally able to figure out a somewhat cleaner way (I feel)..

I start by importing:

import * as OidcModuleAuthProvider from "@connelhooley/react-oidc/dist/AuthProvider";

The part I don't like here is that I have to import the AuthProvider from the dist of your module. However, now that I have that, I am using sinon to stub out the useUser method:

		sinon.stub(OidcModuleAuthProvider, "useUser").returns({
			accessToken: "test",
			id: "test",
			name: "test",
			email: "test",
			role: "test"
		});

And it worked perfectly. I still am unsure about it given I have to jump into /dist. Any thoughts, or is this about the same you'd do?

@connelhooley
Copy link
Owner

Hi @tostringtheory

Glad you're finding the package useful!

Couple of quick disclaimers: this is the first TypeScript or JavaScript NPM package I've authored, and it's the first time I've really unit tested JS code myself.

This is a package I wrote for a project I'm working on in my free time and haven't gotten around to writing unit tests in the project that consumes it just yet.

Looking at your examples I think it makes sense that it only works with the dist import, since the <Authorized> element does a relative import inside the dist folder, and therefore it's import won't be mocked by jest.mock("@connelhooley/react-oidc");.

I was able to get the following to work which is basically what you're already doing:

import React from "react";
import { Authorized } from "@connelhooley/react-oidc";

export function App() {
    return(
        <Authorized><p data-testid="assert">hello</p></Authorized>
    );
}
import React from 'react';
import { render, screen } from '@testing-library/react';
import { useUser } from '@connelhooley/react-oidc/dist/AuthProvider';

import { App } from './App';

jest.mock("@connelhooley/react-oidc/dist/AuthProvider");

const useUserMock = useUser as jest.MockedFunction<typeof useUser>;

test('renders learn react link', async () => {
  useUserMock.mockImplementation(() => ({
    id: "id",
    accessToken: "some token",
    name: "some name",
    email: "some@email.com",
    role: "some role",
}));
  render(<App />);
  expect(await screen.findByTestId("assert")).toHaveTextContent("hello");
});

I really don't like that though so I imagine I'm not following best practices when I publish this package. I think if I used Webpack to merge all the dist JS files into one, the dist bit wouldn't be required, I'll take a look at doing that in the weekend maybe.

@tostringtheory
Copy link
Author

Thanks for the reply @connelhooley . I actually tried code similar to what you had above at first, but it wasn't wanting to work for me at first. I think the reason was at the time I was trying to use Jest, I was still trying to work around not going into dist, and was thus facing issues. Based on what you have above, I'm assuming if I switched mine to mocking the './dist/AuthProvider' instead of the module level, I'd get around the issue I was having at the time.

Also, I believe you're correct, and was one of the last things I discovered before I finally got it to work by mocking the direct dist export, that the relative path in the export was throwing it off...

I've actually been producing internal NPM packages lately, and utilizing Typescript as well with Rollup. The general consensus seems to be that rollup is great for libraries, and webpack is great for apps. I will say that it was extremely low effort on my part to get it implemented. I've also been looking at how the material-ui repository handles their packaging, as they seem to have a very clean build/output format that supports cjs/es/esm through their distributions. They also utilize rollup, but they have some scripts that do some interesting other quirks in their build output, like creating package.json's at every child directory of their default cjs build that points to the esm module declaration with typings information. This gives the really clean import syntax of allowing variations like:

import {  Button } from "@material-ui/core"
//or
import Button from "@material-ui/core/Button"

If you'd like I'd be up to generate a PR with some rollup awesomeness if you're up for it?

@connelhooley
Copy link
Owner

Hey, I was googling it yesterday and it does seem that rollup is the better choice as you suggest.

Thanks for the offer, leave the roll up stuff with me as I'm keen to learn how to use it if that's ok, it seems simple enough on the face of it though! Going to give it a try now.

@tostringtheory
Copy link
Author

Sounds good. I'm going to try and incorporate some of mui's concepts on one of my internal libraries first then, as it seems to provide a great developer experience in consuming the libraries.

Best of luck!

@connelhooley
Copy link
Owner

Hi @tostringtheory

I've bundled the output using rollup.js. Unfortunately I had a load of issues using rollup with TypeScript. Its support for TypeScript seems pretty bad. I came across this error after I followed the rollup docs:

rollup/rollup-plugin-typescript#109

If a project exports interfaces, the recommendation seems to be: use an unofficial plugin and then use babel once that plugin has finished, which seemed quite nasty to me.

I opted to compile using tsc in to a lib folder. This folder contains JS and type definitions. I then use rollup on that lib folder to create Common.JS and ES modules inside the dist folder.

I've placed a "module" property in the package.json, so when webpack loads in the project it should pick up the ES file.

I then installed this package and tried to unit test it. Unfortunately I still couldn't get it to work. I mocked useUser but React said that the <Authorized> component wasn't rendering anything or returned null which makes me think an exception was being thrown somewhere.

I think the following is going to be the best way of mocking it for now. Ultimately mocking out the internals of the package will always be a little risky or hacky so I think mocking the components directly will be better like below:

import React from "react";
import { Authorized } from "@connelhooley/react-oidc";

export function App() {
    return(
        <>
            <Authorized><p data-testid="assert">hello</p></Authorized>
        </>
    );
}
import React from 'react';
import { render, screen } from '@testing-library/react';
import { Authorized, NotAuthorized, Authorizing } from '@connelhooley/react-oidc';

import { App } from './App';

jest.mock("@connelhooley/react-oidc");

const AuthorizedMock = Authorized as jest.MockedFunction<typeof Authorized>;
const NotAuthorizedMock = NotAuthorized as jest.MockedFunction<typeof NotAuthorized>;
const AuthorizingMock = Authorizing as jest.MockedFunction<typeof NotAuthorized>;

test('renders learn react link', async () => {
  AuthorizedMock.mockImplementation((props) => <>{props.children}</>);
  NotAuthorizedMock.mockImplementation(() => <></>);
  AuthorizingMock.mockImplementation(() => <></>);
  render(<App />);
  expect(await screen.findByTestId("assert")).toHaveTextContent("hello");
});

Another option could be for me to write another package for testing that contains a test harness like this:

import React from 'react';
import { render, screen } from '@testing-library/react';
import { AuthTestHarness } from '@connelhooley/react-oidc-test-harness';

import { App } from './App';

jest.mock("@connelhooley/react-oidc");

test('renders learn react link', async () => {
  const user = {
    id: "id",
    accessToken: "some token",
    name: "some name",
    email: "some@email.com",
    role: "some role",
  };
  render(<AuthTestHarness user={user}><App /></AuthTestHarness>);
  expect(await screen.findByTestId("assert")).toHaveTextContent("hello");
});

I've bumped the package up to version 3 as technically the dist imports won't work any more so it's a breaking change.

Let me know how you get on anyway, you may have better luck than me as I'm still pretty new to publishing NPM packages and JS unit testing!

@tostringtheory
Copy link
Author

@connelhooley - good afternoon/evening to you;

I was able to take a look at this briefly on Friday but was otherwise preoccupied.

I want to look a bit more at this this week myself. I don't think publishing a separate library as a test harness is a good solution, just for the added technical debt of maintaining two packages now for 1 library.

The immediate change that I'd suggest making however, is something that just bit me for a good while until I found out the issue with it:

  "main": "lib/index.js",
  "module": "dist/index.es.js",
  "types": "lib/index.d.ts",

In the package.json - you have the main (cjs) file marked as lib/index.js which is not cjs, and freaks Jest out. Could you release a patch update to fix that? I'll poke some more at the rest of it this week and maybe try some stuff out in a fork.

Thanks!

@connelhooley
Copy link
Owner

connelhooley commented Jul 6, 2020

Sorry about that! I've fixed main so that it points at dist/index.js which is a common js module.

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

No branches or pull requests

2 participants