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

Add a sample test to prove tests work when Okta is integrated #129

Open
mraible opened this issue Aug 12, 2020 · 13 comments
Open

Add a sample test to prove tests work when Okta is integrated #129

mraible opened this issue Aug 12, 2020 · 13 comments

Comments

@mraible
Copy link
Contributor

mraible commented Aug 12, 2020

With Okta React 3.0.4, I get a test failure after creating an app and adding the SDK.

TypeError: Cannot read property 'emit' of undefined
  18 | test('renders learn react link', async () => {
  19 |   await act(async () => {
> 20 |     ReactDOM.render(<App/>, container);
     |              ^
  21 |   });
  22 | 
  23 |   const linkElement = container.querySelector('a');
  at AuthService.emit (node_modules/@okta/okta-react/dist/AuthService.js:579:30)
  at AuthService.emitAuthState (node_modules/@okta/okta-react/dist/AuthService.js:182:12)
  at AuthService.clearAuthState (node_modules/@okta/okta-react/dist/AuthService.js:175:12)
  at new AuthService (node_modules/@okta/okta-react/dist/AuthService.js:78:10)
  at node_modules/@okta/okta-react/dist/Security.js:29:33

I don’t get this error with 3.0.1. Proof in https://github.com/mraible/schematics/pull/1.

I tried to modify my test to mock tiny-emitter, but it still doesn’t work.

test('renders learn react link', async () => {
  jest.mock("tiny-emitter", () => {
    return {
      on: jest.fn(),
      off: jest.fn(),
      emit: jest.fn()
    }
  });
  
  await act(async () => {
    ReactDOM.render(<App/>, container);
  });
  
  const linkElement = container.querySelector('a');
  expect(linkElement.textContent).toBe('Learn React');
});
@shuowu
Copy link
Contributor

shuowu commented Aug 12, 2020

@mraible Thanks for adding this ticket to track the issue. Internal Ref: OKTA-322158

@aarongranick-okta
Copy link
Contributor

@swiftone This sounds a lot like another GH issue we have seen. okta/okta-oidc-js#846

@shuowu
Copy link
Contributor

shuowu commented Aug 12, 2020

@aarongranick-okta The change in 3.0.3 changed logic to use emitter from 'auth-js', but looks like for some reason the okta-auth-js dependency is missed from the build bundle or test environment.

Wondering should we just put the auth-js inside the react-sdk's bundle? What's the current practice for angular-sdk?

@swiftone
Copy link
Contributor

We've already had problems with bundling auth-js in the widget, I don't think we want to bundle it more, particularly for okta-react where people can already follow our guides and pull it in independently.

@shuowu
Copy link
Contributor

shuowu commented Aug 13, 2020

@mraible I am successfully reproduced your issue in the sample repo. I also tried to print the authJs instance, then it turns out the test env picked the server bundle instead of the browser one. Below is the log (check the userAgent):

OktaAuth {
      options: {
        issuer: 'https://***.okta.com/oauth2/default',
        httpRequestClient: [Function: fetchRequest],
        storageUtil: {
          getHttpCache: [Function],
          getStorage: [Function],
          storage: [Object]
        },
        headers: undefined
      },
      userAgent: '@okta/okta-react/3.0.4 okta-auth-js-server/3.2.2',
      tx: {
        status: [Function],
        resume: [Function],
        exists: [Function] { _get: [Function] }
      }
    }

@shuowu
Copy link
Contributor

shuowu commented Aug 13, 2020

@mraible It's configurable in jest (https://jest-bot.github.io/jest/docs/configuration.html#browser-boolean) and no mock is needed.

@mraible
Copy link
Contributor Author

mraible commented Aug 13, 2020

@shuowu I tried adding the following to my package.json:

"jest": {
  "browser": true
}

This results in an error:

Out of the box, Create React App only supports overriding these Jest options:

  • clearMocks
  • collectCoverageFrom
  • coveragePathIgnorePatterns
  • coverageReporters
  • coverageThreshold
  • displayName
  • extraGlobals
  • globalSetup
  • globalTeardown
  • moduleNameMapper
  • resetMocks
  • resetModules
  • restoreMocks
  • snapshotSerializers
  • transform
  • transformIgnorePatterns
  • watchPathIgnorePatterns.

These options in your package.json Jest configuration are not currently supported by Create React App:

  • browser

If you wish to override other Jest options, you need to eject from the default setup. You can do so by running npm run eject but remember that this is a one-way operation. You may also file an issue with Create React App to discuss supporting more options out of the box.

All I'm doing is creating a React app with Create React App, then adding our SDK. CRA creates a test as part of the process and as soon as you add Okta's React SDK, that test starts failing.

@shuowu
Copy link
Contributor

shuowu commented Aug 13, 2020

The reason is the test env picked the server bundle which is missing the emitter field in the created instance. And the browser bundle is not supposed to support non-browser env.
Can you try eject or https://github.com/timarney/react-app-rewired (I saw the samples are using this one)?

@aarongranick-okta
Copy link
Contributor

@mraible Here is the jest "browser" config we are using: https://github.com/okta/okta-auth-js/blob/dev-4.0/jest.browser.js

The "moduleNameMapper" lets you map any module name to any path you specify. Because jest is running in node, by default it will pull the node version of the module (the "main" entry in package.json). Webpacked / browser code would use the "browser" entry from the module. https://github.com/okta/okta-auth-js/blob/master/packages/okta-auth-js/package.json#L7

Something like this in your Jest config should work:

moduleNameMapper: {
    '^@okta/okta-auth-js$': '<rootDir>/node_modules/@okta/okta-auth-js/dist/okta-auth-js.min.js'
  }

@mraible
Copy link
Contributor Author

mraible commented Aug 13, 2020

💥 That worked! Adding the following to my package.json fixes the tests.

"jest": {
  "moduleNameMapper": {
    "^@okta/okta-auth-js$": "<rootDir>/node_modules/@okta/okta-auth-js/dist/okta-auth-js.min.js"
  }
}

@robertjd
Copy link
Contributor

robertjd commented Sep 1, 2020

@shuowu do we need to fix this sample with the jest config mentioned above?

@shuowu
Copy link
Contributor

shuowu commented Sep 1, 2020

@robertjd There is no test in this sample repo. The jest config is for tests in Matt's repo.

@mraible
Copy link
Contributor Author

mraible commented Sep 1, 2020

I think it'd be wise to add a test to this repo to show others that this extra step is needed.

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 a pull request may close this issue.

5 participants