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 component tests #163

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Add component tests #163

wants to merge 18 commits into from

Conversation

shaunanoordin
Copy link
Member

PR Overview

This PR adds component tests, to flesh out the Catalog's test suite beyond the absolutely minimal "can the App render without crashing?" test in App.spec.js.

  • Additionally, I'm making a record of what I had to do to 1. setup Jest, and 2. solve specific testing problems, such as mocking modules.

Status

WIP

@shaunanoordin
Copy link
Member Author

Notes on mocking modules:

  • That __mock__ folder (commit 30c6a25) ain't working as expected, and may need to get axed.
  • At the moment, I have to mock every modules used by a component, in the component test itself
    • e.g. App.js uses Panoptes auth, so there's a mocked Panoptes auth module in App.spec.js
  • (Current) rules of thumb:
    • if a dependency fetches data (e.g. Panoptes client), mock it and its responses.
    • if a dependency causes the tests to fail because it or one of its sub-dependencies is written in ESM or CommonJS or whatever it is that Jest doesn't like, mock it.
  • Another acceptable way to mock modules is to look at it straight in the face and declare: I fart in your general direction! Your mother was a hamster and your father smelt of elderberries!

Copy link
Contributor

@eatyourgreens eatyourgreens left a comment

Choose a reason for hiding this comment

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

A couple of comments about ESM. Not sure if they're relevant.

I highly recommend using Nock to mock the Panoptes API, rather than mocking the clients.

}

// Jest doesn't like 'export' (i.e. ES6 modules), so we use module.exports (CommonJS modules).
// Probably because it's running in Node.js.
Copy link
Contributor

@eatyourgreens eatyourgreens Aug 26, 2023

Choose a reason for hiding this comment

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

Node supports export but only for ESM, ie. type: 'module' in package.json or .mjs files.

https://nodejs.org/api/esm.html#enabling

Copy link
Contributor

Choose a reason for hiding this comment

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

This repo's package.json doesn't declare the package type, so Node and Jest default to CommonJS.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks Jim, that's very helpful to know! I didn't realise Node.js could support ESM.

I took some time to explore converting this package to an ESM, but that's gonna be a work in progress.

Copy link
Contributor

Choose a reason for hiding this comment

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

Pure ESM might be a good idea when starting a new project, but it's definitely a fair bit of work converting an existing package.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, #153 is failing the tests because @zooniverse/react-components now depends on d3, for SVG data subjects, and d3 is pure ESM.

src/App/App.spec.js Outdated Show resolved Hide resolved
src/components/SubjectImage/SubjectImage.spec.js Outdated Show resolved Hide resolved
Comment on lines +51 to +53
TODO: this isn't working, as the SubjectImage seems to continue updating outside the act()
Perhaps we need to implement useSWR (https://github.com/zooniverse/community-catalog/pull/131) (to avoid using setState in useEffect), or look into other patterns that handle async calls.
(@shaunanoordin 20230826)
Copy link
Contributor

Choose a reason for hiding this comment

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

React 18 has a new, concurrent data-fetching model. They recommend using a framework like Remix or NextJS, rather than implementing your own data-fetching model (Alice and FEM both have examples of how that can go wrong.)

https://react.dev/blog/2022/03/29/react-v18#suspense-in-data-frameworks

src/App/App.spec.js Outdated Show resolved Hide resolved
@shaunanoordin
Copy link
Member Author

shaunanoordin commented Aug 28, 2023

Updates:

  • General: explored changing the project so it runs as an ESM, but have filed this as a later thing. See Change to ESM (ECMAScript module) #165
  • App.spec.js:
    • The mocked panoptes-client/lib/auth has been made simpler (it ain't an ESModule and is no longer flagged as such)
  • SubjectImage.spec.js
    • The mocked @zooniverse/panoptes-js no longer flagged as an ESModule

Moving forward:

  • I should consider using nock for mocking API responses.
  • However, at the moment, I still need to mock some modules, just to get Jest to run.
    • For example, in SubjectImage.spec.js, if I don't mock @zooniverse/panoptes-js, then Jest will fail at the line import { subjects } from '@zooniverse/panoptes-js' with the error message "Test suite failed to run / Jest encountered an unexpected token"
    • Looking further into the error, I see that this is triggered by community-catalog/node_modules/jose/dist/browser/index.js. Looks like the problem is that the module is written in a format that Jest doesn't like (ESM?), and Jest specifically doesn't automagically transform anything in node_modules into a format that it can work with... so we're on our own here.
    • Things I've looked into and given up on:
      • Make Jest support ESM. (docs) Unfortunately, "Jest ships with experimental support for ECMAScript Modules" and that support has to be enabled with some wonky Node.js flags. Yerp.
      • Use transformIgnorePattern (docs) to tell Jest to, yo, please automagically transform this package in node_modules that's clearly giving you problem. Unfortunately, neither 'transformIgnorePatterns': ['/node_modules/(?!(jose)/)'] nor the more nuclear 'transformIgnorePatterns': [] worked. (The latter did crank my test time from 6s to 30s though, so that affected something. )
    • Other notes:
      • Q: does changing the Catalog to ESM (see 165) help? A: no, alas.
      • Q: Did you try enabling TypeScript support for Jest? (docs) Maybe the problem is jose is written in TypeScript. A: no, I already went cross-eyed trying the other solutions by this point.
Example: error message when `@zooniverse/panoptes-js` module **isn't** mocked, in SubjectImage.spec.js

Results as of commit 47c54aa. Similar results whether we've made those ESM conversions or not.

shaun@Shauns-Zoobook community-catalog % npm run test

> [email protected] test
> jest --config jest.config.js

 PASS  src/components/ProjectContainer/ProjectContainer.spec.js
 FAIL  src/components/SubjectImage/SubjectImage.spec.js
  ● Test suite failed to run

    Jest encountered an unexpected token

    Jest failed to parse a file. This happens e.g. when your code or its dependencies use non-standard JavaScript syntax, or when Jest is not configured to support such syntax.

    Out of the box Jest supports Babel, which will be used to transform your files into valid JS based on your Babel configuration.

    By default "node_modules" folder is ignored by transformers.

    Here's what you can do:
     • If you are trying to use ECMAScript Modules, see https://jestjs.io/docs/ecmascript-modules for how to enable it.
     • If you are trying to use TypeScript, see https://jestjs.io/docs/getting-started#using-typescript
     • To have some of your "node_modules" files transformed, you can specify a custom "transformIgnorePatterns" in your config.
     • If you need a custom transformation specify a "transform" option in your config.
     • If you simply want to mock your non-JS modules (e.g. binary assets) you can stub them out with the "moduleNameMapper" config option.

    You'll find more details and examples of these config options in the docs:
    https://jestjs.io/docs/configuration
    For information about custom transformations, see:
    https://jestjs.io/docs/code-transformation

    Details:

    /Users/shaun/projects/community-catalog/node_modules/jose/dist/browser/index.js:1
    ({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,jest){export { compactDecrypt } from './jwe/compact/decrypt.js';
                                                                                      ^^^^^^

    SyntaxError: Unexpected token 'export'

       9 |  */
      10 |
    > 11 | import { subjects } from '@zooniverse/panoptes-js'
         | ^
      12 |
      13 | export default async function fetchSubject (subjectId) {
      14 |   if (!subjectId) return

      at Runtime.createScriptFromCode (node_modules/jest-runtime/build/index.js:1496:14)
      at Object.<anonymous> (node_modules/@zooniverse/panoptes-js/src/auth.js:1:137)
      at Object.<anonymous> (node_modules/@zooniverse/panoptes-js/src/index.js:2:14)
      at Object.require (src/helpers/fetchSubject.js:11:1)
      at Object.require (src/components/SubjectImage/SubjectImage.js:8:1)
      at Object.require (src/components/SubjectImage/SubjectImage.spec.js:2:1)

 PASS  src/App/App.spec.js

Test Suites: 1 failed, 2 passed, 3 total
Tests:       2 passed, 2 total
Snapshots:   0 total
Time:        6.306 s
Ran all test suites.

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