Fix test suite crashes from nock v14 and ESM incompatibility#132
Fix test suite crashes from nock v14 and ESM incompatibility#132
Conversation
isbinaryfile is ESM-only and cannot be require()'d in Jest's CJS environment. The test was importing it only to immediately replace the export with a jest.fn() mock, so remove the unnecessary require() calls.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
tests/unit/app.test.js
Outdated
| const { isBinaryFile } = require('isbinaryfile'); | ||
|
|
||
| // Mock binary file detection | ||
| // Mock binary file detection (isbinaryfile is ESM-only, so mock directly) |
There was a problem hiding this comment.
CRITICAL: Test is not testing any application code
While the comment explains why the import was removed (ESM-only module), the resulting test has a fundamental flaw: it creates a mock function and then calls the mock directly (lines 180-181), which means it's only testing that the mock returns what it was configured to return.
This is a tautological test that provides no value. The test should either:
- Mock
isbinaryfileat the module level and test actual application code that uses it - Be removed entirely if binary file detection is not actually used in the application
- Be converted to an integration test that tests the actual binary file detection behavior
Currently, this test will always pass regardless of whether binary file detection works correctly in the application.
Code Review SummaryStatus: No New Issues Found | Recommendation: Address existing feedback OverviewReviewed the changes to
The code changes are technically correct with no runtime errors, security issues, or logic bugs. Note: The previously identified issue (test not testing actual application code) remains, but no new issues were introduced by these changes. Files Reviewed (1 file)
|
Address review feedback: the old test was tautological (created a mock
and called it directly). Now uses jest.mock('isbinaryfile') at the
module level, which also fixes the ESM import error, and tests the
binary detection branching logic used by the editgetfile socket handler.
1 participant
Summary
@mswjs/interceptorsinternally, andreplyWithError()emits an async socket error that escapestry/catch, crashing the process after tests complete. Replaced withreply(500)which tests the same failure-handling scenario without the unhandled error.tests/setup.js: Thetypeof require('nock') !== 'undefined'check was always true (require returns the module object). Now usesrequire.cacheto only callcleanAll()when nock was actually loaded by a test.isbinaryfileis ESM-only and cannot berequire()'d in Jest's CJS environment. Removed the unnecessaryrequire()calls since the test only uses ajest.fn()mock.All 5 test suites, 62 tests passing.