fix(tests): repair specs broken by #278 ranking/dream-cycle changes#279
Open
heybeaux wants to merge 1 commit into
Open
fix(tests): repair specs broken by #278 ranking/dream-cycle changes#279heybeaux wants to merge 1 commit into
heybeaux wants to merge 1 commit into
Conversation
- dream-cycle-consolidation.stage.spec.ts: add tx.memory.update mock (production now calls update to mark searchable:true after embedding write) - memory-query-context.service.spec.ts: update sessionId filter assertion to match new session relation OR clause (id | externalId) - memory-job-processor.service.spec.ts: replace $transaction assertion with $executeRawUnsafe — RLS path now sets app.current_account_id directly - prisma-postgres.provider.spec.ts: inject EmbeddingWriteService mock and update embedding assertions to use writeLegacyInlineEmbedding Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR updates four test files to align mock expectations with implementation changes. Changes include extending consolidation transaction mocks with memory update methods, adjusting job processor assertions for RLS context setup, updating query context sessionId filter expectations, and integrating embedding service mocks into provider tests. ChangesTest Expectations Alignment
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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)
src/memory/memory-query-context.service.spec.ts (1)
290-290:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate property check to match new filter shape.
The test checks for the absence of
sessionId, but the implementation now uses asessionrelation filter. When sessionId is omitted, thesessionproperty should not be present in the where clause.🔧 Proposed fix
- expect(sessionCall![0].where).not.toHaveProperty('sessionId'); + expect(sessionCall![0].where).not.toHaveProperty('session');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/memory-query-context.service.spec.ts` at line 290, Update the assertion to reflect the new filter shape: replace the check for absence of 'sessionId' with a check that sessionCall[0].where does not have the 'session' property (i.e., expect(sessionCall![0].where).not.toHaveProperty('session')), so the test verifies the implementation's relation-based filter rather than the old sessionId field.
🧹 Nitpick comments (1)
src/memory/memory-job-processor.service.spec.ts (1)
87-89: ⚡ Quick winOptional: Strengthen the assertion to verify the accountId is included.
The current assertion checks that the SQL contains
SET app.current_account_idbut doesn't verify that the sanitized accountId value (acc-123) is actually included. Consider making the assertion more specific to ensure the full SQL string is correct.💚 More specific assertion
expect(mockPrisma.$executeRawUnsafe).toHaveBeenCalledWith( - expect.stringContaining('SET app.current_account_id'), + expect.stringContaining("SET app.current_account_id = 'acc-123'"), );Or verify the exact string:
expect(mockPrisma.$executeRawUnsafe).toHaveBeenCalledWith( - expect.stringContaining('SET app.current_account_id'), + "SET app.current_account_id = 'acc-123'", );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/memory-job-processor.service.spec.ts` around lines 87 - 89, The test assertion for mockPrisma.$executeRawUnsafe is too generic; update the expectation in memory-job-processor.service.spec.ts to assert the sanitized accountId appears (e.g., 'acc-123') — either by changing expect.stringContaining('SET app.current_account_id') to expect.stringContaining("SET app.current_account_id = 'acc-123'") or by asserting the exact SQL string passed to mockPrisma.$executeRawUnsafe so the test verifies the full SQL includes the accountId value; reference the mockPrisma.$executeRawUnsafe call and the existing expect(...) matcher when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/memory/memory-query-context.service.spec.ts`:
- Line 290: Update the assertion to reflect the new filter shape: replace the
check for absence of 'sessionId' with a check that sessionCall[0].where does not
have the 'session' property (i.e.,
expect(sessionCall![0].where).not.toHaveProperty('session')), so the test
verifies the implementation's relation-based filter rather than the old
sessionId field.
---
Nitpick comments:
In `@src/memory/memory-job-processor.service.spec.ts`:
- Around line 87-89: The test assertion for mockPrisma.$executeRawUnsafe is too
generic; update the expectation in memory-job-processor.service.spec.ts to
assert the sanitized accountId appears (e.g., 'acc-123') — either by changing
expect.stringContaining('SET app.current_account_id') to
expect.stringContaining("SET app.current_account_id = 'acc-123'") or by
asserting the exact SQL string passed to mockPrisma.$executeRawUnsafe so the
test verifies the full SQL includes the accountId value; reference the
mockPrisma.$executeRawUnsafe call and the existing expect(...) matcher when
making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 90537c54-ef94-4185-a032-b9d26847eeb5
📒 Files selected for processing (4)
src/consolidation/stages/dream-cycle-consolidation.stage.spec.tssrc/memory/memory-job-processor.service.spec.tssrc/memory/memory-query-context.service.spec.tssrc/storage/prisma-postgres.provider.spec.ts
Contributor
✅ Recall Benchmark ResultsFull outputCommit: 155fa34 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes 31 unit-test failures introduced by PR #278 (merge commit 825b557). All changes are spec-only — no production code was modified.
Root causes per suite
Test/features —
prisma-postgres.provider.spec.ts(27 failures)PrismaPostgresProvidergained anEmbeddingWriteServiceconstructor dependency in LongMemEval 78.1%: embedding-dim fix, recency/temporal/preference recall, batch-ingest harness #278 to replace direct$executeRawUnsafecalls for embedding writes.EmbeddingWriteServicemock into the test module; updatecreateMemoryandupdateMemoryembedding assertions to assert onwriteLegacyInlineEmbeddinginstead of$executeRawUnsafe.Test/intelligence —
dream-cycle-consolidation.stage.spec.ts(2 failures)consolidateClusternow callstx.memory.update(to marksearchable: trueafter the embedding is written) in addition tocreateandupdateMany. Three test tx mocks were missingupdate, causingTypeError: tx.memory.update is not a function.update: jest.fn().mockResolvedValue({})to all transaction mocks.Test/memory —
memory-query-context.service.spec.ts(1 failure)sessionIdfilter on the SESSION layer changed from a direct{ sessionId }field to a relation join{ session: { OR: [{ id }, { externalId }] } }to support external session IDs.toMatchObjectexpectation to assert the newsession.ORshape.Test/memory —
memory-job-processor.service.spec.ts(1 failure)extractAndEmbedin a$transaction. It now issuesSET app.current_account_id = '...'via$executeRawUnsafethen runs outside any transaction, avoiding pool exhaustion under bulk load.expect($transaction).toHaveBeenCalled()withexpect($executeRawUnsafe).toHaveBeenCalledWith(expect.stringContaining('SET app.current_account_id')).Production bugs found
None. All failures were specs not updated to match intentional behavior changes landed in #278.
Test plan
pnpm jest src/storage/prisma-postgres.provider.spec.ts --forceExit→ 27/27 passpnpm jest src/consolidation/stages/dream-cycle-consolidation.stage.spec.ts --forceExit→ 14/14 passpnpm jest src/memory/memory-query-context.service.spec.ts --forceExit→ 19/19 passpnpm jest src/memory/memory-job-processor.service.spec.ts --forceExit→ 3/3 pass🤖 Generated with Claude Code
Summary by CodeRabbit