test: fix shared mock state and improve test isolation#2023
test: fix shared mock state and improve test isolation#2023SHUBHANSHU602 wants to merge 1 commit intoasyncapi:masterfrom
Conversation
SHUBHANSHU602
requested review from
asyncapi-bot-eve,
derberg,
jonaslagoni and
magicmatatjahu
as code owners
|
What reviewer looks at during PR reviewThe following are ideal points maintainers look for during review. Reviewing these points yourself beforehand can help streamline the review process and reduce time to merge.
|
📝 WalkthroughWalkthroughAdded a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/generator/test/generator.test.js (1)
378-383:⚠️ Potential issue | 🟠 Major
works with an npm packageis missing its cache-hit precondition.Line 382 asserts no install happened, but this test never seeds
utils.__getTemplateDetails. With Line 28 resetting it toundefined, the intended “already installed” branch is not actually arranged.💡 Proposed fix
it('works with an npm package', async () => { utils.__isFileSystemPathValue = false; + utils.__getTemplateDetails = { + pkgPath: '/path/to/template/nameOfTestTemplate', + name: 'nameOfTestTemplate', + version: '1.0.0', + }; const gen = new Generator('nameOfTestTemplate', __dirname); await gen.installTemplate(); expect(arboristMock.reify).not.toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/generator/test/generator.test.js` around lines 378 - 383, The test "works with an npm package" sets utils.__isFileSystemPathValue but never primes the cache condition (utils.__getTemplateDetails) so Generator.installTemplate takes the install path; seed utils.__getTemplateDetails to represent an already-resolved template before calling new Generator/installTemplate (e.g., set utils.__getTemplateDetails to the same template detail object the real code checks) so the code follows the cache-hit branch and arboristMock.reify remains uncalled; update the test to set utils.__getTemplateDetails appropriately and restore/reset it after the test if other tests rely on the undefined default.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/generator/test/generator.test.js`:
- Around line 378-383: The test "works with an npm package" sets
utils.__isFileSystemPathValue but never primes the cache condition
(utils.__getTemplateDetails) so Generator.installTemplate takes the install
path; seed utils.__getTemplateDetails to represent an already-resolved template
before calling new Generator/installTemplate (e.g., set
utils.__getTemplateDetails to the same template detail object the real code
checks) so the code follows the cache-hit branch and arboristMock.reify remains
uncalled; update the test to set utils.__getTemplateDetails appropriately and
restore/reset it after the test if other tests rely on the undefined default.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5bcef845-32f4-4309-a47a-ba1c5db44a84
📒 Files selected for processing (1)
apps/generator/test/generator.test.js
| jest.mock('@asyncapi/parser', () => ({ | ||
| convertToOldAPI: jest.fn((doc) => doc), | ||
| })); |
There was a problem hiding this comment.
@Adi-204 I know it seems like voilating the scope, BUT
While fixing the isolation issue, the tests started reaching the actual parser conversion logic inside lib/parser.js. Previously, because of shared mutable state in the mocks and the way tests were structured, this code path was not always executed consistently.
When the tests began running in a properly isolated state, they attempted to call convertToOldAPI from @asyncapi/parser. However, newer versions of @asyncapi/parser no longer expose this function, which results in the runtime error:
TypeError: convertToOldAPI is not a function
Since the goal of this test suite is to verify the behavior of the Generator class rather than the internals of the parser library, I mocked convertToOldAPI to return the document unchanged. This keeps the tests focused on generator behavior while avoiding failures caused by parser implementation details.
If you prefer, I can also adjust the tests to avoid mocking the parser and instead rely on the actual parser behavior.
There was a problem hiding this comment.
Hey @SHUBHANSHU602, I tried removing the mock locally and ran the tests, and they all passed without any TypeError. So I’m not able to reproduce the issue on my end. Because of that, I’m not sure how the tests are reaching the parser conversion logic in lib/parser.js. Could you share a bit more detail on how you observed this behavior? That would help me better understand the issue and verify it properly.
There was a problem hiding this comment.
@Adi-204 I think I have made the things quite complicated , I will rewrite all the things , to keep the things simple and to avoid scope voilation and confusion.
4 participants
Description
This PR fixes test isolation issues in
apps/generator/test/generator.test.js.Several tests mutate shared state in the mocked
utilsmodule (such as__files,__contentOfFetchedFile,__isFileSystemPathValue, and__getTemplateDetails). Since the mock is loaded once at the module level, these mutations persisted across tests and could lead to unintended side effects between test cases.Changes
beforeEachblock to reset mutable state in theutilsmock before every test.jest.clearAllMocks()to ensure each test starts with a clean state.setTimeout-based assertions with deterministic expectations to avoid async assertions that Jest does not wait for.##Result
**Resolves #1946
Summary by CodeRabbit