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

test: replace jest-mock with sinon, drop expect #427

Closed
wants to merge 1 commit into from

Conversation

gjf7
Copy link
Contributor

@gjf7 gjf7 commented Oct 17, 2024

Follow up #333 .

Remove expect to reduce total number of dependencies, but I found that is hard to use jest-mock without expect, so let's just get rid of it.

const logfile = path.resolve(process.env.NVIM_NODE_LOG_FILE);
fs.writeFileSync(logfile, `${msg}\n`, { flag: 'a' });
}
// const testFile = expect.getState().testPath?.replace(/.*[\\/]/, '');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have't find a way to get the current test filename outside of the mocha test file, maybe it's impossible ?

expect(
await pluginObj.handleRequest('Global', 'function', ['Buffer'])
).not.toEqual(undefined);
expect(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found that pluginObj is not a array, so this test always success before?

Copy link
Member

Choose a reason for hiding this comment

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

don't spend too much time on the "remote plugin" tests, because most of them will be irrelevant after #344

'process',
]);

// expect(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same for this one.


const invalidPaths = nvimRes.invalid.map(i => i.path);
expect(invalidPaths).toEqual(customPaths);
assert.deepStrictEqual(invalidPaths, customPaths);
});

it('searches in additional custom dirs', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

don't know why this test has started failing.

@justinmk
Copy link
Member

This looks like a lateral move in terms of dependency cost so maybe we should skip it.

@gjf7
Copy link
Contributor Author

gjf7 commented Oct 18, 2024

This looks like a lateral move in terms of dependency cost so maybe we should skip it.

Indeed, could merge when we want to drop jest-mock(the work is almost done). expect + jest-mock is fine though.

@gjf7
Copy link
Contributor Author

gjf7 commented Oct 18, 2024

lateral move

Removed expect, dependency count -1 :)

@justinmk
Copy link
Member

Yeah, but the line count increased and some of these look like regressions. E.g.:

-   expect(typeof plugin).toEqual('function');
+   assert(typeof plugin === 'function');
 

should be assert.deepStrictEqual(typeof plugin, 'function');

in order to continue to have ergonomic failure messages.

also deepStrictEqual should be used in most places, because reference-equality (strictEqual) is usually not intended.

@gjf7 gjf7 closed this Oct 22, 2024
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