-
-
Notifications
You must be signed in to change notification settings - Fork 11
Refactor: Bring everything up to speed with new deps and node 20 #80
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
Conversation
* Based on works of John McBride (@jpmcb) * Update dependencies: + Bump: + @typescript-eslint/eslint-plugin: ^6.7.0 + @typescript-eslint/parser: ^6.7.0 + eslint: ^8.49.0 + typescript: ^5.2.2 + Add: + eslint-import-resolver-typescript: ^3.6.0 Required for eslint to work with typescript import resolver + msw: ^1.3.1 Introduced to replace nock + Replace deprecated: + Replace @zeit/ncc with @vercel/ncc, upstream suggestion Build error: digital envelope routines::unsupported + Drop: + nock due to the issue [1] related to the fetch experimental feature of nodejs * Refactor code: + TypeScript compile errors + ESLint errors and warnings + Tests reimplement GitHub API responses mockup using msw/node instead of nock * Declare the action runs using node20 --- [1] nock/nock#2397 Signed-off-by: Neutron Soutmun <[email protected]>
Signed-off-by: Neutron Soutmun <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow. Excellent work: on quick preliminary review, this looks great. This will take me some time to go over and approve, but looks like all tests pass.
Bummer that nock
won't work for our use cases but msw
looks like a suitable replacement.
Any updates on this effort? I am interested in this action |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @neutronth - I'll take this wholesale and followup with any additional changes before cutting a new release. Thanks again! Huge effort!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request refactors the test suite to replace the deprecated nock library with msw and updates other dependencies to support Node 20. Key changes include:
- Removal of nock and conversion of HTTP mocking to msw using setupServer and rest handlers.
- Replacement of assertions on nock interceptors with the new observeReq helper.
- Consistent updates across multiple test files to align with new dependency versions and Node 20 support.
Reviewed Changes
Copilot reviewed 49 out of 50 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
tests/issueCommentTest/assign.test.ts | Replaces nock calls with msw handlers for issue assignment tests. |
tests/cronJobTest/lgtm.test.ts | Converts cron job merge tests from nock to msw and updates method assertions. |
tests/issueCommentTest/uncc.test.ts | Updates tests for uncc commands to use msw. |
tests/cronJobTest/labelPr.test.ts | Converts label PR tests from nock to msw with observeReq for label verification. |
... | Similar consistent replacements in tests for cc, retitle, close, lock, etc. |
Files not reviewed (1)
- .eslintrc.json: Language not supported
Comments suppressed due to low confidence (3)
tests/labelPr.test.ts:29
- Ensure that using an encoded path (.github%2Flabels.yaml) matches the expected GitHub API format. If the API expects '/.github/labels.yaml' instead, the endpoint should be adjusted accordingly.
rest.get(`${utils.api}/repos/Codertocat/Hello-World/contents/.github%2Flabels.yaml`, utils.mockResponse(200, labelFileContents))
tests/issueCommentTest/assign.test.ts:52
- [nitpick] Verify that the 'observeReq.called()' promise reliably resolves in all tests to ensure that the msw handlers are properly triggering the expected HTTP requests.
await observeReq.called()
tests/issueCommentTest/uncc.test.ts:39
- [nitpick] Consider using more descriptive variable names for different instances of 'observeReq' if multiple are declared in the same test block to reduce potential confusion.
const observeReq = new utils.observeRequest()
* Refactor: Bring everything up to speed with new deps and node 20 * Based on works of John McBride (@jpmcb) * Update dependencies: + Bump: + @typescript-eslint/eslint-plugin: ^6.7.0 + @typescript-eslint/parser: ^6.7.0 + eslint: ^8.49.0 + typescript: ^5.2.2 + Add: + eslint-import-resolver-typescript: ^3.6.0 Required for eslint to work with typescript import resolver + msw: ^1.3.1 Introduced to replace nock + Replace deprecated: + Replace @zeit/ncc with @vercel/ncc, upstream suggestion Build error: digital envelope routines::unsupported + Drop: + nock due to the issue [1] related to the fetch experimental feature of nodejs * Refactor code: + TypeScript compile errors + ESLint errors and warnings + Tests reimplement GitHub API responses mockup using msw/node instead of nock * Declare the action runs using node20 --- [1] nock/nock#2397 Signed-off-by: Neutron Soutmun <[email protected]> * chore: cosmetic apply with prettier Signed-off-by: Neutron Soutmun <[email protected]> --------- Signed-off-by: Neutron Soutmun <[email protected]>
Refactor: Bring everything up to speed with new deps and node 20
Required for eslint to work with typescript import resolver
Introduced to replace nock
Build error: digital envelope routines::unsupported
feature of nodejs
of nock
[1] nock/nock#2397