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

Switch from esmock to node.js mock tracker #446

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

Conversation

albertshay888
Copy link

converted all tests that uses esmock to Node.js MockTracker

@fraxken
Copy link
Member

fraxken commented Jan 10, 2025

Hi @albertshay888, can you remove the tests commented (what you migrated) ? Thanks

test/httpServer.test.js Outdated Show resolved Hide resolved
test/commands/scorecard.test.js Outdated Show resolved Hide resolved
@PierreDemailly
Copy link
Member

mock.module is for v23 only. Please use mock.method instead; it'll work for mocking Node.js APIs. We'll probably migrate to mock.module when v24 will be active (Nov 2025) (we don't support odd-numbered releases as they don't enter LTS).

Also, you don't need to do dynamic imports after mocking, you can drop these and import what needs to be tested classically at the top of the file.

…js and added the imports at the top of each file
Copy link
Member

@PierreDemailly PierreDemailly left a comment

Choose a reason for hiding this comment

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

You can also uninstall esmock (npm un esmock) and change the test script from
glob -c \"node --loader=esmock --no-warnings --test-concurrency 1 --test\" \"test/**/*.test.js\"
to
glob -c \"node --test-concurrency 1 --test\" \"test/**/*.test.js\"

Note that there are little conflicts, you can sync your fork and rebase with the main branch

test/commands/scorecard.test.js Outdated Show resolved Hide resolved
test/commands/scorecard.test.js Outdated Show resolved Hide resolved
test/commands/scorecard.test.js Outdated Show resolved Hide resolved
test/httpServer.test.js Outdated Show resolved Hide resolved
test/httpServer.test.js Outdated Show resolved Hide resolved
test/httpServer.test.js Outdated Show resolved Hide resolved
test/httpServer.test.js Outdated Show resolved Hide resolved
test/httpServer.test.js Outdated Show resolved Hide resolved
test/httpServer.test.js Outdated Show resolved Hide resolved
@Yaziidev
Copy link

Is this ok or can i do it ?

@PierreDemailly
Copy link
Member

Is this ok or can i do it ?

I guess you can work on it 😄

…ck imports and previous commented out tests, removed readFileSyncMock.mock.restoreAll() in the last test for scorecard.test.js file, fixed all async spacing, and removed indexModule and used buildServerimport instead
@PierreDemailly
Copy link
Member

Hi @albertshay888 Can you please rebase and fix conflicts ?
it looks good but we cannot run the CI

@albertshay888
Copy link
Author

albertshay888 commented Jan 24, 2025

in latest commit- rebase and fix conflicted, screenshots of terminal view below:

terminal view - rebase and fixed conflicts terminal view - everything up to date

@PierreDemailly
Copy link
Member

@albertshay888
The problem is that your fork master branch in not synced:
image

You have to sync it (you should have a Sync button on GitHub), then rebase

@albertshay888
Copy link
Author

no longer has conflicts with base branch

@PierreDemailly
Copy link
Member

Tests KO

Copy link
Member

@PierreDemailly PierreDemailly left a comment

Choose a reason for hiding this comment

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

Also make sure tests pass locally

@@ -70,8 +70,6 @@
"c8": "^10.1.2",
"cross-env": "^7.0.3",
"esbuild": "^0.24.0",
"eslint-plugin-jsdoc": "^50.6.2",
Copy link
Member

Choose a reason for hiding this comment

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

This dep is needed for the eslint config, you removed it when resolving conflicts i guess (just reinstall it)

Copy link
Author

Choose a reason for hiding this comment

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

reinstalled "eslint-plugin-jsdoc": "^50.6.2", in latest commit

Comment on lines 105 to 106
].join("\n")
);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
].join("\n")
);
].join("\n"));

Copy link
Author

Choose a reason for hiding this comment

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

fixed in latest commit

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.

4 participants