Revise and expand block update tests #1120
Conversation
WalkthroughThe test suite for Changes
Sequence Diagram(s)sequenceDiagram
participant TestSuite
participant BlockRepository
participant DB (Mocked)
TestSuite->>BlockRepository: preUpdateMany(movedBlockIds)
BlockRepository->>DB: find({ _id: { $in: movedBlockIds } })
BlockRepository->>DB: find({ _id: { $nin: movedBlockIds }, nextBlocks: { $in: movedBlockIds } })
BlockRepository->>BlockRepository: prepareBlocksInCategoryUpdateScope(...)
BlockRepository->>BlockRepository: prepareBlocksOutOfCategoryUpdateScope(...)
BlockRepository-->>TestSuite: (Assertions on update calls)
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
api/src/chat/repositories/block.repository.spec.ts (3)
208-243: Circular-reference test passes but mock completeness can be improvedSolid scenario, but
jest.spyOn(blockRepository, 'find')is left dangling without restoring the original implementation, which could leak into following tests if any of them rely onfind’s real behaviour.
AddmockRestore()(or move the spy insideafterEach) to keep the suite isolated.226 .mockResolvedValue({} as any); + // Ensure isolation + afterEach(() => { + (blockRepository.find as jest.Mock).mockRestore(); + });
342-355: ObjectId strictness causes secondfindexpectation to fail
preUpdateManybuilds the second query with string IDs, whereas the test expectsTypes.ObjectIdinstances, leading to the pipeline failure at line 348.Unless the production code converts to
ObjectIds, replace the strict equality with a looser matcher:- expect(mockFind).toHaveBeenNthCalledWith(2, { - _id: { $nin: objectIds }, + expect(mockFind).toHaveBeenNthCalledWith(2, { + _id: { $nin: validIds }, category: movedBlocks[0].category, $or: [ - { attachedBlock: { $in: objectIds } }, - { nextBlocks: { $in: objectIds } }, + { attachedBlock: { $in: validIds } }, + { nextBlocks: { $in: validIds } }, ], });or use
expect.arrayContainingto stay implementation-agnostic.
This will unblock the failing assertion without weakening coverage.🧰 Tools
🪛 GitHub Actions: workflow 1837 of branch chore/add-unit-tests-for-blocks-category-updates
[error] 348-348: Test failure: Expected mockFind to have been called with specific arguments on the 2nd call, but received different arguments.
10-10: UnusedTypesimport once expectations are loosenedIf the previous comment is applied and you stop converting to
ObjectId, theTypesimport becomes dead code – drop it to keep the spec tidy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/src/chat/repositories/block.repository.spec.ts(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
api/src/chat/repositories/block.repository.spec.ts (1)
api/src/chat/repositories/block.repository.ts (2)
prepareBlocksInCategoryUpdateScope(171-194)prepareBlocksOutOfCategoryUpdateScope(204-221)
🪛 GitHub Actions: workflow 1837 of branch chore/add-unit-tests-for-blocks-category-updates
api/src/chat/repositories/block.repository.spec.ts
[error] 306-306: Test failure: Expected mockUpdateOne not to have been called, but it was called once.
[error] 348-348: Test failure: Expected mockFind to have been called with specific arguments on the 2nd call, but received different arguments.
| it('should not update blocks if their references are not in the moved ids', async () => { | ||
| const otherBlocks = [ | ||
| { | ||
| id: blockValidIds[1], | ||
| attachedBlock: 'some-other-id', | ||
| nextBlocks: [], | ||
| }, | ||
| { | ||
| id: blockValidIds[2], | ||
| attachedBlock: null, | ||
| nextBlocks: ['some-other-id-2'], | ||
| }, | ||
| ] as Block[]; | ||
| const mockUpdateOne = jest.spyOn(blockRepository, 'updateOne'); | ||
|
|
||
| await blockRepository.prepareBlocksOutOfCategoryUpdateScope(otherBlocks, [ | ||
| blockValidIds[0], | ||
| ]); | ||
|
|
||
| expect(mockUpdateOne).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Failing assertion: implementation updates nextBlocks even when nothing changes
The pipeline error at line 306 stems from this expectation.
prepareBlocksOutOfCategoryUpdateScope currently performs
if (nextBlocks?.length) { updateOne(...) }meaning it will always run updateOne when nextBlocks is non-empty – even if no element was removed.
Your test (correctly) assumes we should skip the write when the payload is identical.
Either:
- Change the repository implementation to avoid redundant writes (preferred):
- const nextBlocks = block.nextBlocks?.filter(
- (nextBlock) => !ids.includes(nextBlock),
- );
-
- if (nextBlocks?.length) {
+ const filteredNextBlocks = block.nextBlocks?.filter(
+ (nextBlock) => !ids.includes(nextBlock),
+ );
+
+ if (
+ filteredNextBlocks &&
+ filteredNextBlocks.length !== block.nextBlocks.length
+ ) {
await this.updateOne(block.id, { nextBlocks: filteredNextBlocks });
}- Or relax the expectation in this test to allow one call.
Choose 1 if you want to avoid unnecessary DB writes.
🧰 Tools
🪛 GitHub Actions: workflow 1837 of branch chore/add-unit-tests-for-blocks-category-updates
[error] 306-306: Test failure: Expected mockUpdateOne not to have been called, but it was called once.
🤖 Prompt for AI Agents
In api/src/chat/repositories/block.repository.spec.ts around lines 287 to 307,
the test expects no calls to updateOne when nextBlocks remain unchanged, but the
current implementation calls updateOne whenever nextBlocks is non-empty. To fix
this, modify the prepareBlocksOutOfCategoryUpdateScope method to compare the
existing nextBlocks with the new nextBlocks and only call updateOne if there is
an actual change, thereby preventing redundant database writes.
Enhance the test coverage for block updates, including handling circular references and ensuring blocks are not updated if their references are not in the moved IDs.
Summary by CodeRabbit