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

Change testing framework from Mocha to Jest #719

Merged
merged 6 commits into from
Apr 10, 2024
Merged

Conversation

pradeepnschrodinger
Copy link
Collaborator

@pradeepnschrodinger pradeepnschrodinger commented Mar 30, 2024

Description

I'm replacing Mocha with Jest.

Motivation and Context

Jest vs Mocha

Jest already comes packed with assertions, stubs, mocks, and snapshot testing features.
I'm planning to add some snapshot testing later, and maintaining two testing frameworks seem like a pain.

With Mocha, we also had to add Chai and Sinon to the stack.
Morever, it doesn't support snapshot testing out of the box.

Security vulnerabilities

I also had trouble fixing security issues cause of mocha-webpack and mocha-loader, cause they haven't released newer versions in a while.
We've also been using a beta version of mocha-webpack which doesn't work with newer versions of mocha...
And I don't think anyones been using these cause they appear broken since forever...

A bonus is that the following vulnerability should now be resolved since mocha will be removed.

flat vulnerable to Prototype Pollution
mocha@6.2.3 requires flat@^4.1.0 via yargs-unparser@1.6.0
The earliest fixed version is 5.0.1.

Severity: Critical (9.8 / 10)

Extra changes

I've bumped up the node versions in our CI workflow from [12, 14, 16] to [14, 16, 18, 20].
Node v12 reached EoL on 2022, and v14 reach EoL last year.

How Has This Been Tested?

Ran yarn run test and verified all tests pass.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.

@wiz-inc-87ff8274df
Copy link

wiz-inc-87ff8274df bot commented Mar 30, 2024

Wiz Scan Summary

IaC Misconfigurations 0C 0H 0M 0L 0I
Vulnerabilities 0C 1H 3M 0L 0I
Sensitive Data 0C 0H 0M 0L 0I
Total 0C 1H 3M 0L 0I
Secrets 0🔑

@@ -18,7 +18,7 @@ jobs:
# run this job for every combination of given matrix variables
strategy:
matrix:
node-version: [12.x, 14.x, 16.x]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

12 is pretty old. Kicked it out, and included 18 and 20 in its place.

@@ -1,8 +1,7 @@
/**
* Copyright Schrodinger, LLC
*/
import { assert } from 'chai';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

assertions are included in jest's expect

@@ -1,8 +1,7 @@
/**
* Copyright Schrodinger, LLC
*/
import { assert } from 'chai';
import sinon from 'sinon';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

mocks/spies are included in jest

@@ -33,7 +46,7 @@ describe('FixedDataTableRoot', function () {
renderer.render(table);
let tableRender = renderer.getRenderOutput();

assert.isTrue(isElement(tableRender));
expect(isElement(tableRender)).toBe(true);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here and elsewhere: assert() calls are replaced with expect() calls

renderTable({ scrollLeft: undefined });
});
}).not.toThrow();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

expect(...).not lets you negate the expectations

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed cause it doesn't work and also cause it relies on mocha-loader

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This file isn't needed anymore because we have jest-environment-jsdom, which setups a DOM environment for our tests.

@karry08
Copy link
Collaborator

karry08 commented Apr 9, 2024

what is this wiz scan summary

assert.equal(
table.getTableState().scrollX,
300,
'should set scrollX to 300'
Copy link
Collaborator

Choose a reason for hiding this comment

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

no way to pass this parameter?

Copy link
Collaborator Author

@pradeepnschrodinger pradeepnschrodinger Apr 10, 2024

Choose a reason for hiding this comment

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

Ah good catch.
Unfortunately there doesn't seem to be a clean and simple enough native way in JEST to do this.

There's some workarounds in https://stackoverflow.com/a/68332874, like wrapping the individual checks inside another it like it("should set scrollX to 300"), or to use the https://github.com/mattphillips/jest-expect-message package.

But they all seem like an overkill at this point.
We can look into this in a separate PR if anyone reports they're missing the custom error messages.

@karry08
Copy link
Collaborator

karry08 commented Apr 9, 2024

lgtm

@pradeepnschrodinger pradeepnschrodinger merged commit 134b18b into master Apr 10, 2024
12 checks passed
@pradeepnschrodinger pradeepnschrodinger deleted the jest-testing branch April 10, 2024 23:50
@pradeepnschrodinger
Copy link
Collaborator Author

Released with v2.0.9.

@pradeepnschrodinger pradeepnschrodinger added the dependencies Pull requests that update a dependency file label Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants