test: replace jest with mocha #420

gjf7 · 2024-10-11T15:59:20Z

Closes #333.

To make a transition from Jest feasible. Capabilities Jest offers that we might need to keep:

Expect

Expect is a standalone npm package.

Mock

The jest.fn(), jest.spyOn() is provided by jest-mock, we can just use this package directly.

gjf7 · 2024-10-11T16:19:48Z

@justinmk Mocha doesn't support coverage out of box, Do u think https://github.com/istanbuljs/nyc is a good choice for us?

justinmk · 2024-10-11T17:20:46Z

@justinmk Mocha doesn't support coverage out of box, Do u think https://github.com/istanbuljs/nyc is a good choice for us?

nyc is deprecated, https://github.com/bcoe/c8 is the current best option AFAIK.

gjf7 · 2024-10-16T13:35:36Z

packages/neovim/package.json

-    "test-build": "npm test --runInBand --coverage",
-    "test-staged": "npm test --bail --no-cache --findRelatedTests",
+    "test": "mocha --exit --parallel --require ts-node/register --require src/testSetup.ts src/**/*.test.ts",
+    "test-coverage": "c8 --reporter=json --reporter=html npm test",


test-build make no sense here, so I rename it to test-coverage

gjf7 · 2024-10-16T13:36:14Z

packages/neovim/package.json

-    "test-staged": "npm test --bail --no-cache --findRelatedTests",
+    "test": "mocha --exit --parallel --require ts-node/register --require src/testSetup.ts src/**/*.test.ts",
+    "test-coverage": "c8 --reporter=json --reporter=html npm test",
+    "test-staged": "npm test --bail",


Not sure we still need this script command ?

It's for husky or something like that.

node-client/package.json

Lines 44 to 49 in 4d6d4d3

"lint-staged": {

"*.{ts,js}": [

"eslint --fix",

"npm run test-staged"

]

},

I'd probably prefer to remove lint-staged and similar stuff. This repo is simple and should stay simple, so not worth having that lint-staged dep imo.

But probably off-topic for this PR.

gjf7 · 2024-10-16T14:07:54Z

C8 requires node >= 18, wtf?

justinmk

I was a bit worried about this but this looks like a win. You did it in a way that reduces risk and churn. Thank you!

justinmk · 2024-10-16T14:04:00Z

packages/neovim/package.json

+    "@types/node": "16.9.x",
+    "c8": "^10.1.2",
+    "expect": "^29.7.0",
+    "jest-mock": "^29.7.0",


jest-mock is fine as a temporary step (if it saves some noise in this PR), but maybe we want to switch to https://sinonjs.org/ ? Or maybe it doesn't matter. (But maybe it's confusing to have both jest and mocha :)

packages/neovim/package.json

justinmk · 2024-10-16T14:05:24Z

.eslintrc.js

@@ -70,6 +71,7 @@ module.exports = {

    'import/extensions': 'off',
    'import/prefer-default-export': 'off',
+    "import/no-extraneous-dependencies": ["error", {"devDependencies": true, "optionalDependencies": false, "peerDependencies": false}],


single quotes

justinmk · 2024-10-16T14:06:23Z

packages/integration-tests/package.json

-    "testPathIgnorePatterns": [".d.ts", ".js"]
+    "@types/mocha": "^10.0.9",
+    "c8": "^10.1.2",
+    "expect": "^29.7.0",


OK as a transitional step, but can probably be avoided in a later PR?

I'll handle this(replace expect and jest-mock with assert and sinon) in a follow up PR :)

I'm thinking that although expect made by Jest, should we replace it ? or is it worth to do so(expect is a good assert lib in my opinion)?

justinmk · 2024-10-16T14:08:33Z

packages/neovim/src/testUtil.ts

+// eslint-disable-next-line import/no-mutable-exports
+export let proc: cp.ChildProcessWithoutNullStreams;
+// eslint-disable-next-line import/no-mutable-exports
+export let nvim: NeovimClient;


can/should this be a function? it's a bit "magical" as a global var, and also makes debugging a bit more painful?

Not a blocker.

I'll handle this too in a follow up PR :)

packages/neovim/src/host/NvimPlugin.test.ts

justinmk · 2024-10-16T14:13:25Z

C8 requires node >= 18, wtf?

We could skip codecov for older node.

gjf7 · 2024-10-16T15:15:33Z

packages/neovim/src/attach/attach.test.ts

@@ -155,7 +150,7 @@ describe('Nvim API', () => {
  });

  it('emits "disconnect" after quit', done => {


don't know why this test is keep failing!

we can skip it for now. even after the 'exit' workaround it still fails for me locally, sometimes.

#419

gjf7 · 2024-10-16T15:50:10Z

looks like there is a Uncaught error in our test util.

justinmk · 2024-10-16T15:52:50Z

maybe drop --parallel for now? not sure if these tests are prepared for that.

gjf7 · 2024-10-16T15:59:38Z

maybe drop --parallel for now? not sure if these tests are prepared for that.

Drop --parallel make `process.env.NODE_ENV = 'test' not executed, don't know why.

Got error Exception during run: TypeError: Cannot read properties of undefined (reading 'parseVersion' NODE_ENV is undefined.

justinmk · 2024-10-16T16:16:21Z

that's weird, but based on mochajs/mocha#185 seems like mocha intentionally does not set NODE_ENV? For now we could just add to testSetup.ts:

process.env.NODE_ENV = 'test';

gjf7 · 2024-10-16T16:24:15Z

packages/neovim/src/testSetup.ts


-import * as testUtil from './testUtil';
-// import * as jest from '@jest/globals'
+process.env.NODE_ENV = 'test';


Already done that, I mean this line is not working after drop --parallel.

gjf7 · 2024-10-16T17:11:40Z

Turns out we must set 'NODE_ENV=test' in .mocharc.js in non-parallel mode. Sets in testSetup is useless :)

gjf7 · 2024-10-16T17:13:00Z

Need to check why codecov is't triggered.

justinmk · 2024-10-16T18:12:21Z

might need a .c8rc.json in packages/neovim/. IIRC the "lcov" reporter was important to get it to work with codecov.

example (untested, just a sample):

{
    "report-dir": "../../coverage/neovim",
    "reporter": ["lcov"],
    "all": true,
    "include": ["src/**"],
    "exclude": [
        "src/test/**"
    ]
}

and the GHA workflow:

  uses: codecov/codecov-action@v4
  with:
      verbose: true
      file: ./coverage/neovim/lcov.info
      token: ${{ secrets.CODECOV_TOKEN }}

gjf7 · 2024-10-17T00:40:38Z

package.json

@@ -30,7 +30,7 @@
    "dev": "npm run --stream --parallel dev --workspaces --if-present",
    "publish:neovim": "cd packages/neovim && cp ../../README.md . && cp ../../CHANGELOG.md . && npm publish",
    "test": "npm run test --workspaces --if-present",
-    "test-coverage": "npm run test --workspaces --if-present",
+    "test-coverage": "npm run test-coverage --workspaces --if-present",


My bad :), codecov works now.

gjf7 · 2024-10-17T00:47:18Z

Follow up PR TODO:

eliminate the magical global nvim, proc in test util.
replace expect with Node.js built-in assert module to reduces total number of dependencies

.eslintrc.js

justinmk · 2024-10-17T08:25:59Z

replace jest-mock with sinon

On second thought, let's keep jest-mock for now. Mocking should be rare, and unless sinon improves something, using jest-mock avoids churn and doesn't really hurt.

keep expect is fine

reading https://jestjs.io/docs/expect , mocha (or assert) already provides this except the syntax is different? If removing expect reduces total number of dependencies, then it might be worth doing.

eliminate the magical global nvim, proc in test util

I'm also not sure about this. The current situation looks ok, since this repo is very simple (and getting simpler, thanks to this PR :).

gjf7 · 2024-10-17T09:23:55Z

Ready for merge :)

gjf7 force-pushed the jest-to-mocha branch 2 times, most recently from 457321d to ac97e71 Compare October 16, 2024 13:55

gjf7 commented Oct 16, 2024

View reviewed changes

gjf7 marked this pull request as ready for review October 16, 2024 13:56

gjf7 force-pushed the jest-to-mocha branch from ac97e71 to fe3b87d Compare October 16, 2024 14:02

gjf7 marked this pull request as draft October 16, 2024 14:03

justinmk approved these changes Oct 16, 2024

View reviewed changes

test: replace jest with mocha

234b0f7

gjf7 force-pushed the jest-to-mocha branch from fe3b87d to 234b0f7 Compare October 16, 2024 15:09

gjf7 commented Oct 16, 2024

View reviewed changes

gjf7 marked this pull request as ready for review October 16, 2024 15:16

skip "emits disconnect after quit" test

8a8bc2e

gjf7 commented Oct 16, 2024

View reviewed changes

gjf7 added 3 commits October 17, 2024 00:45

drop --parallel

265382e

set NODE_ENV on test script

c562c62

add .mocharc.js to make NODE_ENV='test' work

5352703

gjf7 force-pushed the jest-to-mocha branch from 5c28750 to 5352703 Compare October 16, 2024 17:08

enable coverage

a694093

gjf7 commented Oct 17, 2024

View reviewed changes

justinmk reviewed Oct 17, 2024

View reviewed changes

.eslintrc.js Outdated Show resolved Hide resolved

fix eslint rule

510f1cb

gjf7 force-pushed the jest-to-mocha branch from cc2b993 to 510f1cb Compare October 17, 2024 09:14

justinmk merged commit 63fb96c into neovim:master Oct 17, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: replace jest with mocha #420

test: replace jest with mocha #420

gjf7 commented Oct 11, 2024 •

edited

Loading

gjf7 commented Oct 11, 2024 •

edited

Loading

justinmk commented Oct 11, 2024

gjf7 Oct 16, 2024

gjf7 Oct 16, 2024

justinmk Oct 16, 2024 •

edited

Loading

gjf7 commented Oct 16, 2024

justinmk left a comment

justinmk Oct 16, 2024 •

edited

Loading

justinmk Oct 16, 2024

justinmk Oct 16, 2024

gjf7 Oct 16, 2024

gjf7 Oct 16, 2024

justinmk Oct 16, 2024 •

edited

Loading

gjf7 Oct 16, 2024

justinmk commented Oct 16, 2024

gjf7 Oct 16, 2024

justinmk Oct 16, 2024 •

edited

Loading

gjf7 commented Oct 16, 2024

justinmk commented Oct 16, 2024 •

edited

Loading

gjf7 commented Oct 16, 2024 •

edited

Loading

justinmk commented Oct 16, 2024

gjf7 Oct 16, 2024

gjf7 commented Oct 16, 2024

gjf7 commented Oct 16, 2024

justinmk commented Oct 16, 2024 •

edited

Loading

gjf7 Oct 17, 2024

gjf7 commented Oct 17, 2024 •

edited

Loading

justinmk commented Oct 17, 2024 •

edited

Loading

gjf7 commented Oct 17, 2024

	"lint-staged": {
	"*.{ts,js}": [
	"eslint --fix",
	"npm run test-staged"
	]
	},

		@@ -155,7 +150,7 @@ describe('Nvim API', () => {
		});

		it('emits "disconnect" after quit', done => {

test: replace jest with mocha #420

test: replace jest with mocha #420

Conversation

gjf7 commented Oct 11, 2024 • edited Loading

Expect

Mock

gjf7 commented Oct 11, 2024 • edited Loading

justinmk commented Oct 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmk Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

gjf7 commented Oct 16, 2024

justinmk left a comment

Choose a reason for hiding this comment

justinmk Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmk Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmk commented Oct 16, 2024

Choose a reason for hiding this comment

justinmk Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

gjf7 commented Oct 16, 2024

justinmk commented Oct 16, 2024 • edited Loading

gjf7 commented Oct 16, 2024 • edited Loading

justinmk commented Oct 16, 2024

Choose a reason for hiding this comment

gjf7 commented Oct 16, 2024

gjf7 commented Oct 16, 2024

justinmk commented Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

gjf7 commented Oct 17, 2024 • edited Loading

justinmk commented Oct 17, 2024 • edited Loading

gjf7 commented Oct 17, 2024

gjf7 commented Oct 11, 2024 •

edited

Loading

gjf7 commented Oct 11, 2024 •

edited

Loading

justinmk Oct 16, 2024 •

edited

Loading

justinmk Oct 16, 2024 •

edited

Loading

justinmk Oct 16, 2024 •

edited

Loading

justinmk Oct 16, 2024 •

edited

Loading

justinmk commented Oct 16, 2024 •

edited

Loading

gjf7 commented Oct 16, 2024 •

edited

Loading

justinmk commented Oct 16, 2024 •

edited

Loading

gjf7 commented Oct 17, 2024 •

edited

Loading

justinmk commented Oct 17, 2024 •

edited

Loading