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

[TASK-1046] Add unit test for component #5120

Open
wants to merge 11 commits into
base: beta
Choose a base branch
from

Conversation

pauloamorimbr
Copy link

@pauloamorimbr pauloamorimbr commented Sep 19, 2024

Checklist

  1. If you've added code that should be tested, add tests
  2. If you've changed APIs, update (or create!) the documentation
  3. Ensure the tests pass
  4. Make sure that your code lints and that you've followed our coding style
  5. Write a title and, if necessary, a description of your work suitable for publishing in our release notes
  6. Mention any related issues in this repository (as #ISSUE) and in other repositories (as kobotoolbox/other#ISSUE)
  7. Open an issue in the docs if there are UI/UX changes

Description

This commit creates the base configuration and samples for adding unit tests with Jest.
Simple tests were created for the Button and KoboSelect3 components as samples.

Notes

  • Some new packages were added for jest to work
  • Configuration for jest was added into /jsapp/jest
  • 2 new scripts were created on package.json:
    • npm run jest
    • npm run jest-watch (Very useful when writing tests)
    • Both scripts can filter test files as in: npm run jest-watch -- Button
  • To avoid confusion with mocha .tests.* file, we're using the .spec.* extension for tests.
  • A test for the Button component was added checking for its interaction when enabled and disabled
  • A test for the KoboSelect3 component was added checking for its mouse and keyboard interaction

@pauloamorimbr pauloamorimbr changed the base branch from main to beta September 19, 2024 19:09
@pauloamorimbr pauloamorimbr marked this pull request as ready for review September 20, 2024 03:45
@pauloamorimbr pauloamorimbr changed the title [TASK 1046] Add unit test for component [TASK-1046] Add unit test for component Sep 20, 2024
Copy link

package.json Outdated Show resolved Hide resolved
@@ -159,6 +168,7 @@
"swc-loader": "^0.2.6",
"terser-webpack-plugin": "^5.3.10",
"ts-loader": "^9.5.1",
"ts-node": "^10.9.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

📓 So ts-node lets us:

  • write jest.config.ts in TypeScript instead of JavaScript
  • launch it from npx jest --config ./jsapp/jest/jest.config.ts without an extra step

It's not strictly necessary but it's in the spirit of leveraging TypeScript. In the future we could apply this to Webpack. I looked into it:

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add "ts-node": { "swc": true } to tsconfig.json.

Since ts-node has to transpile the config, it does affect test startup time a little bit.

time npm run jest
As-is jest.config.ts 2021 ms ± 23 ms
With swc: true jest.config.ts 1156 ms ± 79 ms
Plain JS config jest.config.mjs 989 ms ± 40 ms
benchmark

hyperfine --warmup 3 'npm run jest'

TypeScript, ts-node
  Time (mean ± σ):      2.021 s ±  0.023 s    [User: 3.671 s, System: 0.255 s]
  Range (min … max):    1.987 s …  2.066 s    10 runs

TypeScript, ts-node, `"ts-node": { "swc": true },`
  Time (mean ± σ):      1.156 s ±  0.079 s    [User: 1.454 s, System: 0.168 s]
  Range (min … max):    1.091 s …  1.285 s    10 runs

Plain JS (jest.config.mjs)
  Time (mean ± σ):     989.7 ms ±  39.7 ms    [User: 1254.6 ms, System: 129.9 ms]
  Range (min … max):   959.0 ms … 1092.9 ms    10 runs

package.json Show resolved Hide resolved
@@ -201,7 +211,9 @@
"copy-fonts": "python3 ./scripts/copy_fonts.py && npm run generate-icons",
"hint": "node ./scripts/hints.js",
"storybook": "storybook dev -p 6006 --no-open",
"build-storybook": "storybook build"
"build-storybook": "storybook build",
"jest": "jest --config ./jsapp/jest/jest.config.ts",
Copy link
Contributor

Choose a reason for hiding this comment

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

We could add this to our GitHub CI (.github/workflows/npm-test.yml)

      # Run Jest component tests
      - name: Run Jest component tests
      - run: npx jest --config ./jsapp/jest/jest.config.ts --ci

Here, or in a separate PR.

Copy link

Choose a reason for hiding this comment

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

Good catch, I think that's in the scope of adding jest to project, because normally all tests are always run on CI.

Copy link
Author

Choose a reason for hiding this comment

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

I was thinking about doing this in a different PR, after some more tests were added. But I believe there's no problem doing so here anyways. 👍🏻

{value: '3', label: 'Avocado'},
];

// A wrapper is needed for the component to retain value changes
Copy link
Contributor

Choose a reason for hiding this comment

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

📓 Good point — many of our Kobo components are like this — 'controlled'-only. It'd be possible to make them more flexible.

'Course, then there would be more to test 😅

Copy link
Author

Choose a reason for hiding this comment

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

It's a common practice I found for single component testing, since most of the components will depend on external context anyways. So I really don't think it's the case of making them 'uncontrolled' just for the sake of tests. We only need to be careful to not create biased tests that could influence the component behavior. (but yeah, cool article! 😄)

@pauloamorimbr
Copy link
Author

Cool. Checks added the new tests already!

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.

3 participants