feat: bulk db operations #404
Conversation
petermasking
changed the title
petermasking
changed the title
WalkthroughThe pull request introduces bulk operations to the database integration. Two new methods, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Database
participant Driver
Client->>Database: updateRecords(type, query, data)
Database->>Driver: updateRecords(type, query, data)
Driver-->>Database: Promise<void>
Database-->>Client: Promise<void>
sequenceDiagram
participant Client
participant Database
participant Driver
Client->>Database: deleteRecords(type, query)
Database->>Driver: deleteRecords(type, query)
Driver-->>Database: Promise<void>
Database-->>Client: Promise<void>
Assessment against linked issues
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/integrations/database/Database.ts(1 hunks)src/integrations/database/definitions/interfaces.ts(1 hunks)src/integrations/database/implementations/memory/Memory.ts(1 hunks)src/integrations/database/implementations/mongodb/MongoDb.ts(1 hunks)test/integrations/database/fixtures/queries.fixture.ts(1 hunks)test/integrations/database/implementation.spec.ts(3 hunks)
🧰 Additional context used
🧬 Code Definitions (4)
src/integrations/database/definitions/interfaces.ts (3)
src/integrations/database/implementations/memory/Memory.ts (4)
type(289-306)query(206-213)query(215-232)data(308-324)src/integrations/database/definitions/types.ts (3)
RecordType(6-6)RecordQuery(20-20)RecordData(11-11)src/integrations/database/implementations/mongodb/MongoDb.ts (2)
query(209-250)data(295-318)
src/integrations/database/Database.ts (3)
src/integrations/database/implementations/memory/Memory.ts (4)
type(289-306)query(206-213)query(215-232)data(308-324)src/integrations/database/definitions/types.ts (3)
RecordType(6-6)RecordQuery(20-20)RecordData(11-11)src/integrations/database/implementations/mongodb/MongoDb.ts (2)
query(209-250)data(295-318)
src/integrations/database/implementations/memory/Memory.ts (3)
src/integrations/database/definitions/types.ts (2)
QueryStatement(19-19)RecordData(11-11)src/integrations/database/errors/RecordNotUpdated.ts (1)
RecordNotUpdated(4-10)src/integrations/database/errors/RecordNotFound.ts (1)
RecordNotFound(4-10)
test/integrations/database/implementation.spec.ts (2)
test/integrations/database/fixtures/queries.fixture.ts (1)
QUERIES(8-84)src/integrations/database/definitions/types.ts (1)
RecordData(11-11)
🔇 Additional comments (3)
test/integrations/database/fixtures/queries.fixture.ts (1)
10-10: LGTM: The new query definition follows the existing pattern.The new
UPDATEDquery is well-structured and follows the existing pattern in this file, providing a clean way to test the new bulk update functionality.src/integrations/database/definitions/interfaces.ts (1)
16-17: LGTM: Interface extensions align with the PR objectives.The new method signatures for bulk operations follow the same pattern as the single-record operations and maintain consistency with the overall interface design.
src/integrations/database/Database.ts (1)
68-71: LGTM: The deleteRecords implementation is correct.The implementation correctly delegates to the driver's deleteRecords method with the appropriate parameters.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/integrations/database/implementations/memory/Memory.ts (1)
135-144: Bulk updates mirror single updates.Similar to
.deleteRecords(), consider a single-pass approach for large data sets. Repeatedly calling.updateRecord()can degrade performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/integrations/database/Database.ts(2 hunks)src/integrations/database/implementations/memory/Memory.ts(4 hunks)test/integrations/database/implementation.spec.ts(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
test/integrations/database/implementation.spec.ts (6)
test/integrations/database/fixtures/records.fixture.ts (2)
RECORD_TYPES(4-8)RECORDS(10-24)test/integrations/database/fixtures/queries.fixture.ts (1)
QUERIES(8-84)test/integrations/database/fixtures/values.fixture.ts (1)
VALUES(2-24)src/integrations/database/errors/RecordNotFound.ts (1)
RecordNotFound(4-10)src/integrations/database/errors/RecordNotUpdated.ts (1)
RecordNotUpdated(4-10)src/integrations/database/definitions/types.ts (1)
RecordData(11-11)
src/integrations/database/implementations/memory/Memory.ts (3)
src/integrations/database/errors/RecordNotFound.ts (1)
RecordNotFound(4-10)src/integrations/database/definitions/types.ts (2)
QueryStatement(19-19)RecordData(11-11)src/integrations/database/errors/RecordNotUpdated.ts (1)
RecordNotUpdated(4-10)
src/integrations/database/Database.ts (2)
src/integrations/database/definitions/types.ts (6)
RecordType(6-6)RecordId(7-7)RecordQuery(20-20)RecordField(9-9)RecordSort(23-23)RecordData(11-11)src/integrations/utilities/sanitize.ts (1)
sanitize(16-26)
🔇 Additional comments (20)
test/integrations/database/implementation.spec.ts (6)
4-4: Import usage looks proper.No concerns regarding the newly introduced import of
RecordData; it aligns with the test usage below.
36-54: Good test coverage for bulk deletion.These tests cover both the successful bulk delete scenario and the no-match scenario, ensuring robust functionality.
71-86: Clear verification for limiting results.The suite properly checks the limit and offset functionality, ensuring
.limitNumberOfRecordsbehaves as intended.
88-114: Thorough coverage for record reading.The tests validate reading a full record, partial fields, and handling of a non-existing record. Nicely done.
267-284: Comprehensive one-record update testing.These tests correctly verify updating a record by ID and handling a non-existing record. Looks good.
286-307: Well-structured tests for batch updates.The suite ensures all matching records are updated correctly and checks that no error is thrown when none match, providing meaningful coverage.
src/integrations/database/Database.ts (8)
18-21: Straightforward.clear()pass-through.No issues; the method simply delegates to the driver and maintains consistency.
23-26:.connect()delegation is clear.This direct pass-through ensures the driver determines the connection state.
35-38: Consistent.deleteRecord()method.Delegating the call to the driver is appropriate. No concerns.
40-43: Bulk deletion logic is properly delegated.Implementation aligns with the driver’s
deleteRecordsinterface, keeping the method succinct.
45-48: Clean.disconnect()delegation.Method is consistent with the driver-based disconnection approach.
55-59: Direct.readRecord()pass-through.No issues; it cleanly calls
this.#driver.readRecord(...).
65-71: Sanitization in.updateRecord()looks good.Ensuring data is sanitized before delegating to the driver helps maintain consistency with single-record updates.
72-77: Bulk update method includes sanitization.This addresses past concerns about missing data sanitization in
updateRecords. Implementation is now consistent with.updateRecord().src/integrations/database/implementations/memory/Memory.ts (6)
35-38:.clear()method is straightforward.Clearing the in-memory store is well-handled.
40-43:.connect()implementation is simple.No issues; flipping the
#connectedflag to true is sufficient for an in-memory driver.
58-69: Single-record deletion logic is correct.Throwing
RecordNotFoundwhen no index is found ensures correctness. This is in line with the tests.
82-85: Simple disconnection approach.Setting
#connected = falseis enough for an in-memory driver.
94-105: Reading a record by ID is well-implemented.The logic checks for existence and throws
RecordNotFoundwhen absent, matching the test scenarios.
119-133: One-record update approach is sound.Updates the desired fields and throws
RecordNotUpdatedfor non-existing records. No concerns.
basmasking
requested review from
petermasking
and removed request for
basmasking
* Memory integration optimizations * New bulk operation errors * Code formatting and consistence improvements
petermasking
requested review from
basmasking
and removed request for
petermasking
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/integrations/database/Database.ts (2)
40-43: Consider adding JSDoc comments for better documentation.The
findRecordmethod is well-implemented but could benefit from JSDoc comments explaining its purpose, parameters, and return value for better maintainability and developer experience.+/** + * Finds a single record matching the provided query. + * + * @param type - The type of records to search in + * @param query - The query to filter records + * @param fields - Optional fields to include in the result + * @param sort - Optional sort criteria + * @returns A promise that resolves to the matching record or undefined if not found + */ findRecord(type: RecordType, query: RecordQuery, fields?: RecordField[], sort?: RecordSort): Promise<RecordData | undefined> { return this.#driver.findRecord(type, query, fields, sort); }
45-48: Consider adding JSDoc comments for better documentation.Similar to the
findRecordmethod,searchRecordscould benefit from JSDoc comments explaining its purpose, parameters, and return value.+/** + * Searches for records matching the provided query. + * + * @param type - The type of records to search in + * @param query - The query to filter records + * @param fields - Optional fields to include in the result + * @param sort - Optional sort criteria + * @param limit - Optional maximum number of records to return + * @param offset - Optional number of records to skip + * @returns A promise that resolves to an array of matching records + */ searchRecords(type: RecordType, query: RecordQuery, fields?: RecordField[], sort?: RecordSort, limit?: number, offset?: number): Promise<RecordData[]> { return this.#driver.searchRecords(type, query, fields, sort, limit, offset); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/integrations/database/Database.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/integrations/database/Database.ts (2)
src/integrations/database/definitions/types.ts (6)
RecordType(6-6)RecordQuery(20-20)RecordField(9-9)RecordSort(23-23)RecordData(11-11)RecordId(7-7)src/integrations/utilities/sanitize.ts (1)
sanitize(16-26)
🔇 Additional comments (3)
src/integrations/database/Database.ts (3)
57-62: Good job implementing data sanitization for bulk updates.The
updateRecordsmethod properly sanitizes the data before passing it to the driver, maintaining consistency with theupdateRecordmethod's approach to data handling.It would be helpful to add JSDoc comments for better documentation:
+/** + * Updates multiple records matching the provided query. + * + * @param type - The type of records to update + * @param query - The query to filter records for updating + * @param data - The data to update in the matching records + * @returns A promise that resolves when the update is complete + */ updateRecords(type: RecordType, query: RecordQuery, data: RecordData): Promise<void> { const cleanData = sanitize(data); return this.#driver.updateRecords(type, query, cleanData); }
64-67: Implementation looks good.The
deleteRecordmethod correctly delegates to the driver's implementation.Consider adding JSDoc comments for better documentation:
+/** + * Deletes a single record by its ID. + * + * @param type - The type of record to delete + * @param id - The ID of the record to delete + * @returns A promise that resolves when the deletion is complete + */ deleteRecord(type: RecordType, id: RecordId): Promise<void> { return this.#driver.deleteRecord(type, id); }
69-72: Implementation looks good.The
deleteRecordsmethod correctly delegates to the driver's implementation for bulk deletion.Consider adding JSDoc comments for better documentation:
+/** + * Deletes multiple records matching the provided query. + * + * @param type - The type of records to delete + * @param query - The query to filter records for deletion + * @returns A promise that resolves when the deletion is complete + */ deleteRecords(type: RecordType, query: RecordQuery): Promise<void> { return this.#driver.deleteRecords(type, query); }
There was a problem hiding this comment.
The tests are ordered in alphabetical order, but the order should be similar to the database defintion for easier maintenance.
| { | ||
| it('should update all records matching the query', async () => | ||
| { | ||
| const data: RecordData = { size: 40 }; |
There was a problem hiding this comment.
In the values fixture we put the values we use in the tests. These hard coded numbers should be avoided in the test to make them easier to maintain.
There was a problem hiding this comment.
data added to values.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
test/integrations/database/implementation.spec.ts (4)
16-30: Consider testing multiple-match scenarios for.findRecord.These tests effectively cover the “first match found” and “no match” cases. If multiple records can match, it may be beneficial to test ensuring the method indeed returns a single record (and specifically the first).
180-197: Tests for.updateRecordare thorough.These tests verify both the successful update scenario and the error path for non-existent records. Consider adding a scenario for updating multiple fields at once, if applicable, to broaden coverage.
199-220: Good coverage for.updateRecords.The tests confirm successful bulk updates and the no-match scenario. One optional enhancement: verify that records not covered by the query remain unchanged, ensuring updates are narrowly applied.
278-307: Extensive coverage for.sortRecords.Tests verify ascending, descending, and multi-field variations. As an optional improvement, consider adding coverage for cases where some records might lack the sort field.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/integrations/database/fixtures/values.fixture.ts(1 hunks)test/integrations/database/implementation.spec.ts(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/integrations/database/implementation.spec.ts (7)
test/integrations/database/fixtures/records.fixture.ts (2)
RECORD_TYPES(4-8)RECORDS(10-24)test/integrations/database/fixtures/queries.fixture.ts (1)
QUERIES(8-84)src/integrations/database/errors/RecordNotUpdated.ts (1)
RecordNotUpdated(4-10)src/integrations/database/definitions/types.ts (1)
RecordData(11-11)src/integrations/database/errors/RecordNotFound.ts (1)
RecordNotFound(4-10)test/integrations/database/fixtures/sorts.fixture.ts (1)
SORTS(5-11)test/integrations/database/fixtures/results.fixture.ts (1)
RESULTS(8-33)
🔇 Additional comments (4)
test/integrations/database/fixtures/values.fixture.ts (1)
23-27: Good addition of a dedicatedSIZEproperty.Adding a new
SIZEproperty in the fixtures helps keep test data consistent and centralized. This aligns well with previous feedback about avoiding hard-coded constants directly in the tests.test/integrations/database/implementation.spec.ts (3)
4-4: Type import looks correct.Importing
RecordDataas a type-only import is a clean approach, reducing overhead while improving clarity of the test logic.
222-239: Robust tests for.deleteRecord.These tests correctly validate normal and error scenarios (record not found). Concurrency or partial-field checks are likely out of scope here, so this coverage suffices.
241-259: Comprehensive.deleteRecordstesting.Both the successful bulk-deletion and the empty-match scenario are properly verified, ensuring no accidental failures or unclear edge cases.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/integrations/database/implementation.spec.ts (1)
180-307: Consider reordering test suites to match database class structure.The test suites appear to be in a somewhat random order. According to a previous review comment (line 1), the tests should be ordered similar to the database definition for easier maintenance.
Consider reordering the test suites to match the order of methods in the Database class:
.readRecord.findRecord.searchRecords.limitNumberOfRecords.sortRecords.updateRecord.updateRecords.deleteRecord.deleteRecords
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/integrations/database/implementation.spec.ts(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/integrations/database/implementation.spec.ts (7)
test/integrations/database/fixtures/records.fixture.ts (2)
RECORD_TYPES(4-8)RECORDS(10-24)test/integrations/database/fixtures/queries.fixture.ts (1)
QUERIES(8-84)test/integrations/database/fixtures/values.fixture.ts (1)
VALUES(2-29)src/integrations/database/errors/RecordNotUpdated.ts (1)
RecordNotUpdated(4-10)src/integrations/database/definitions/types.ts (1)
RecordData(11-11)src/integrations/database/errors/RecordNotFound.ts (1)
RecordNotFound(4-10)test/integrations/database/fixtures/sorts.fixture.ts (1)
SORTS(5-11)
🔇 Additional comments (5)
test/integrations/database/implementation.spec.ts (5)
4-4: LGTM: Added RecordData import.The import of RecordData type is appropriate as it's used in the new test cases.
202-210: LGTM: Comprehensive test for updateRecords.The test correctly verifies both the count and the actual values of the updated records, ensuring that the bulk update functionality works as expected. This addresses a previous review comment.
212-219: LGTM: Edge case handling for updateRecords.Good test for ensuring that no errors are thrown when no records match the query. This aligns with the behavior we would expect and addresses a previous review comment.
243-249: LGTM: Comprehensive test for deleteRecords.The test correctly verifies that all matching records are deleted by checking that a subsequent search returns no results.
251-258: LGTM: Edge case handling for deleteRecords.Good test for ensuring that no errors are thrown when no records match the query, and that the database state remains unchanged. This addresses a previous review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
test/integrations/database/implementation.spec.ts (3)
219-226: Enhance the.updateRecordstest with proper value usage from fixtures.The test is implementing the suggestions from previous reviews, which is good. However, you should use the value from the fixtures consistently.
- const data: RecordData = VALUES.SIZE; + const data: RecordData = VALUES.SIZE; await database.updateRecords(RECORD_TYPES.PIZZAS, QUERIES.EQUALS, data); const records = await database.searchRecords(RECORD_TYPES.PIZZAS, QUERIES.UPDATED); expect(records).toHaveLength(2); - expect(records[0].size).toBe(40); - expect(records[1].size).toBe(40); + // Use the fixture value instead of hardcoding + expect(records[0].size).toBe(VALUES.SIZE.size); + expect(records[1].size).toBe(VALUES.SIZE.size);
230-234: Use fixture values for consistency in the second updateRecords test.To maintain consistency with the rest of the tests and follow the feedback in previous reviews, use values from fixtures rather than hardcoded values.
- const data: RecordData = { size: 99 }; + // Add this value to VALUES.SIZE in the fixture, e.g., NO_MATCH_SIZE: { size: 99 } + const data: RecordData = VALUES.SIZE.NO_MATCH_SIZE; // This should not throw an error await expect(database.updateRecords(RECORD_TYPES.PIZZAS, QUERIES.NO_MATCH, data)) .resolves.toBeUndefined();
293-323: Maintain consistent indentation.The indentation for the
.sortRecordstest suite appears to have changed. Ensure it matches the style of other test suites for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/integrations/database/implementation.spec.ts(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/integrations/database/implementation.spec.ts (8)
test/integrations/database/fixtures/records.fixture.ts (2)
RECORD_TYPES(4-8)RECORDS(10-24)test/integrations/database/fixtures/queries.fixture.ts (1)
QUERIES(8-84)test/integrations/database/fixtures/values.fixture.ts (1)
VALUES(2-29)src/integrations/database/errors/RecordNotFound.ts (1)
RecordNotFound(4-10)src/integrations/database/errors/RecordNotUpdated.ts (1)
RecordNotUpdated(4-10)src/integrations/database/definitions/types.ts (1)
RecordData(11-11)test/integrations/database/fixtures/sorts.fixture.ts (1)
SORTS(5-11)test/integrations/database/fixtures/results.fixture.ts (1)
RESULTS(8-33)
🔇 Additional comments (2)
test/integrations/database/implementation.spec.ts (2)
196-255: Clarify intent regarding single-record operations.The PR is introducing bulk operations with
updateRecordsanddeleteRecords, but the test file still contains tests for the single-record operationsupdateRecordanddeleteRecord. Please confirm if:
- Both single and bulk operations will coexist (preferred for backward compatibility)
- Or if the single operations are deprecated and these tests should be removed
If single operations are being maintained, consider adding a note in the tests explaining the relationship between the single and bulk operations.
267-274: LGTM - Properly implemented.deleteRecordstest with verification.The test correctly implements the suggested behavior from past review comments, including verification that the database state remains unchanged when no records match the query.
| describe('.findRecord', () => | ||
| { | ||
| it('should delete a record by id', async () => | ||
| { | ||
| const id = RECORDS.FRUITS.APPLE.id as string; | ||
|
|
||
| await database.deleteRecord(RECORD_TYPES.FRUITS, id); | ||
|
|
||
| const promise = database.readRecord(RECORD_TYPES.FRUITS, id); | ||
| await expect(promise).rejects.toStrictEqual(new RecordNotFound()); | ||
| }); | ||
|
|
||
| it('should throw an error when the record cannot be deleted', async () => | ||
| it('should return the first matched record', async () => | ||
| { | ||
| const promise = database.deleteRecord(RECORD_TYPES.PIZZAS, VALUES.IDS.NON_EXISTING); | ||
| await expect(promise).rejects.toStrictEqual(new RecordNotFound()); | ||
| const result = await database.findRecord(RECORD_TYPES.PIZZAS, QUERIES.EMPTY); | ||
| expect(result).toMatchObject(RECORDS.PIZZAS.MARGHERITA); | ||
| }); | ||
| }); | ||
|
|
||
| describe('.updateRecord', () => | ||
| { | ||
| it('should update a record by id', async () => | ||
| it('should return undefined when no match found', async () => | ||
| { | ||
| const id = RECORDS.FRUITS.PEAR.id as string; | ||
|
|
||
| await database.updateRecord(RECORD_TYPES.FRUITS, id, { country: VALUES.UPDATES.COUNTRY }); | ||
|
|
||
| const result = await database.readRecord(RECORD_TYPES.FRUITS, id); | ||
| expect(result?.country).toBe(VALUES.UPDATES.COUNTRY); | ||
| const result = await database.findRecord(RECORD_TYPES.PIZZAS, QUERIES.NO_MATCH); | ||
| expect(result).toBe(undefined); | ||
| }); | ||
|
|
||
| it('should throw an error when record cannot be updated', async () => | ||
| it('should throw an error when a record is not found', async () => | ||
| { | ||
| const promise = database.updateRecord(RECORD_TYPES.FRUITS, VALUES.IDS.NON_EXISTING, {}); | ||
| await expect(promise).rejects.toStrictEqual(new RecordNotUpdated()); | ||
| const promise = database.readRecord(RECORD_TYPES.PIZZAS, VALUES.IDS.NON_EXISTING); | ||
| await expect(promise).rejects.toStrictEqual(new RecordNotFound()); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Fix duplicate test suite for .findRecord.
There are two identical test suites for .findRecord which will result in the same tests being run twice. The first suite also includes a test case that belongs to .readRecord since it's testing database.readRecord() behavior.
Apply this fix:
// Keep the second suite at lines 61-74 and remove the first one at lines 40-59
- describe('.findRecord', () =>
- {
- it('should return the first matched record', async () =>
- {
- const result = await database.findRecord(RECORD_TYPES.PIZZAS, QUERIES.EMPTY);
- expect(result).toMatchObject(RECORDS.PIZZAS.MARGHERITA);
- });
-
- it('should return undefined when no match found', async () =>
- {
- const result = await database.findRecord(RECORD_TYPES.PIZZAS, QUERIES.NO_MATCH);
- expect(result).toBe(undefined);
- });
-
- it('should throw an error when a record is not found', async () =>
- {
- const promise = database.readRecord(RECORD_TYPES.PIZZAS, VALUES.IDS.NON_EXISTING);
- await expect(promise).rejects.toStrictEqual(new RecordNotFound());
- });
- });Then, move the third test case to the .readRecord suite:
describe('.readRecord', () =>
{
// Existing tests...
+
+ it('should throw an error when a record is not found', async () =>
+ {
+ const promise = database.readRecord(RECORD_TYPES.PIZZAS, VALUES.IDS.NON_EXISTING);
+ await expect(promise).rejects.toStrictEqual(new RecordNotFound());
+ });
});Also applies to: 61-74
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/integrations/database/implementation.spec.ts (1)
40-59:⚠️ Potential issueFix duplicate test suite for
.findRecord.There are two identical test suites for
.findRecordwhich will result in the same tests being run twice. The first suite also includes a test case that belongs to.readRecordsince it's testingdatabase.readRecord()behavior.Apply this fix:
// Keep the second suite at lines 61-74 and remove the first one at lines 40-59 - describe('.findRecord', () => - { - it('should return the first matched record', async () => - { - const result = await database.findRecord(RECORD_TYPES.PIZZAS, QUERIES.EMPTY); - expect(result).toMatchObject(RECORDS.PIZZAS.MARGHERITA); - }); - - it('should return undefined when no match found', async () => - { - const result = await database.findRecord(RECORD_TYPES.PIZZAS, QUERIES.NO_MATCH); - expect(result).toBe(undefined); - }); - - it('should throw an error when a record is not found', async () => - { - const promise = database.readRecord(RECORD_TYPES.PIZZAS, VALUES.IDS.NON_EXISTING); - await expect(promise).rejects.toStrictEqual(new RecordNotFound()); - }); - });Then, move the third test case to the
.readRecordsuite:describe('.readRecord', () => { // Existing tests... + + it('should throw an error when a record is not found', async () => + { + const promise = database.readRecord(RECORD_TYPES.PIZZAS, VALUES.IDS.NON_EXISTING); + await expect(promise).rejects.toStrictEqual(new RecordNotFound()); + }); });Also applies to: 61-74
🧹 Nitpick comments (1)
test/integrations/database/implementation.spec.ts (1)
273-273: Consider using dynamic assertion for record count.Rather than hardcoding the expected number of records (5), consider retrieving the count dynamically before running the delete operation or storing it in a constant in the test fixtures.
- expect(records).toHaveLength(5); + const expectedCount = Object.keys(RECORDS.PIZZAS).length; + expect(records).toHaveLength(expectedCount);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/integrations/database/fixtures/values.fixture.ts(1 hunks)test/integrations/database/implementation.spec.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/integrations/database/fixtures/values.fixture.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/integrations/database/implementation.spec.ts (7)
test/integrations/database/fixtures/records.fixture.ts (1)
RECORDS(10-24)test/integrations/database/fixtures/queries.fixture.ts (1)
QUERIES(8-84)test/integrations/database/fixtures/values.fixture.ts (1)
VALUES(2-34)src/integrations/database/errors/RecordNotFound.ts (1)
RecordNotFound(4-10)src/integrations/database/errors/RecordNotUpdated.ts (1)
RecordNotUpdated(4-10)src/integrations/database/definitions/types.ts (1)
RecordData(11-11)test/integrations/database/fixtures/sorts.fixture.ts (1)
SORTS(5-11)
🔇 Additional comments (5)
test/integrations/database/implementation.spec.ts (5)
4-4: Added RecordData import for bulk operations.The RecordData import is properly added to support the new bulk operations test suites.
215-236: Well-structured updateRecords test suite.Good implementation of the updateRecords test suite covering both successful updates and the case when no records match. The tests verify both the count of matching records and that the updated values are correct.
219-220: Good use of fixtures for test data.Using VALUES.SIZE fixture instead of hardcoded values improves maintainability, following the project's convention as mentioned in past reviews.
257-274: Well-structured deleteRecords test suite.The test suite for deleteRecords is well-designed, covering both successful deletions and the case when no records match. The verification that records are actually deleted and that the operation doesn't throw errors when nothing matches is comprehensive.
294-323: Unchanged sortRecords test suite.The sortRecords test suite appears unchanged in functionality, with only line number changes due to the insertion of the new test suites.
3 participants
Fixes #401
Changes proposed in this pull request:
@MaskingTechnology/comify
Summary by CodeRabbit
New Features
Bug Fixes
Tests