Conversation
…s, JSON serialization, and schema evolution.
…rToJson` to use Dart 3 pattern matching syntax.
…ble` dependencies.
…t` to `integration_test/all_tests.dart`.
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 16 files with indirect coverage changes 🚀 New features to boost your workflow:
|
…in the test generation tool.
…ited index/id query tests.
…values, nested embedded values, and null checks.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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 (2)
packages/isar_plus_test/tool/generate_long_double_test.dart (1)
13-23: ⚡ Quick winReplacement order issue makes line 19 ineffective.
Line 18 replaces
'filter_int_test', which is a substring of'filter_int_list_test'. This means'filter_int_list_test'becomes'filter_long_list_test'at line 18, making the replacement at line 19 redundant since the pattern no longer exists in the string.While the final output is correct, this creates confusing and fragile logic. Consider either:
- Solution 1: Remove line 18 entirely for the list test generation (line 19 handles the complete replacement)
- Solution 2: Reverse the order—replace the more specific pattern (
filter_int_list_test) before the general substring (filter_int_test)♻️ Proposed fix: Remove redundant replacement
final intListTestFile = File('test/filter/filter_int_list_test.dart'); final intListTest = intListTestFile.readAsStringSync(); final longListTest = intListTest .replaceAll('Int', 'Long') .replaceAll('short', 'int') - .replaceAll('filter_int_test', 'filter_long_test') .replaceAll('filter_int_list_test', 'filter_long_list_test') .replaceAll('intModels', 'longModels'); File( 'test/filter/filter_long_list_test.dart', ).writeAsStringSync(longListTest);🤖 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 `@packages/isar_plus_test/tool/generate_long_double_test.dart` around lines 13 - 23, The replacement order in the longListTest generation is fragile: remove the redundant .replaceAll('filter_int_test', 'filter_long_test') call (the .replaceAll('filter_int_list_test', 'filter_long_list_test') already performs the intended change) so update the longListTest chain in generate_long_double_test.dart to omit that specific replacement and keep the others as-is.packages/isar_plus_test/test/enum_test.dart (1)
342-345: ⚡ Quick winStrengthen migrated-data assertions in “Added value” test.
Line 343 currently checks only length; it doesn’t validate enum decoding behavior for pre-existing rows after reopening with
EnumModelV2. Add explicit assertions for IDs1and2so this test actually guards compatibility semantics.Suggested patch
// Existing data should still be readable final results = isar2.enumModelV2s.where().findAll(); expect(results.length, 2); + expect(isar2.enumModelV2s.get(1)!.byteEnum, ByteEnum2.option1); + expect(isar2.enumModelV2s.get(2)!.byteEnum, ByteEnum2.option1);🤖 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 `@packages/isar_plus_test/test/enum_test.dart` around lines 342 - 345, The test only asserts results.length == 2 but must also verify that the migrated enum values decode correctly for existing rows: after reopening with EnumModelV2, query isar2.enumModelV2s.where().findAll() (the variable results) and add explicit assertions that the entry with id 1 has the expected enum value and the entry with id 2 has its expected enum value (use the EnumModelV2 enum accessor on the returned objects); update the test to locate the objects by id (e.g., results.firstWhere((e) => e.id == 1)) and assert their enum fields equal the expected EnumModelV2.<expectedMember> and similarly for id 2.
🤖 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.
Inline comments:
In `@packages/isar_plus_test/test/enum_test.dart`:
- Around line 393-401: The test currently relies on the unstable order returned
by isar2.enumModels.where().findAll() and indexes results[0]/results[1]; change
the assertions to fetch by id instead (use isar2.enumModels.get(1) and
isar2.enumModels.get(2)) and assert their byteEnum values directly so the
migrated-row checks are stable; update the two expect calls that reference
results[...] to use the corresponding get(id) values and keep the same expected
enums (ByteEnum.option2 for id 1 and ByteEnum.option1 for id 2).
---
Nitpick comments:
In `@packages/isar_plus_test/test/enum_test.dart`:
- Around line 342-345: The test only asserts results.length == 2 but must also
verify that the migrated enum values decode correctly for existing rows: after
reopening with EnumModelV2, query isar2.enumModelV2s.where().findAll() (the
variable results) and add explicit assertions that the entry with id 1 has the
expected enum value and the entry with id 2 has its expected enum value (use the
EnumModelV2 enum accessor on the returned objects); update the test to locate
the objects by id (e.g., results.firstWhere((e) => e.id == 1)) and assert their
enum fields equal the expected EnumModelV2.<expectedMember> and similarly for id
2.
In `@packages/isar_plus_test/tool/generate_long_double_test.dart`:
- Around line 13-23: The replacement order in the longListTest generation is
fragile: remove the redundant .replaceAll('filter_int_test', 'filter_long_test')
call (the .replaceAll('filter_int_list_test', 'filter_long_list_test') already
performs the intended change) so update the longListTest chain in
generate_long_double_test.dart to omit that specific replacement and keep the
others as-is.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3bbc207c-f80b-4684-9b69-7fccc8bf88ad
📒 Files selected for processing (11)
.github/workflows/test.yamlpackages/isar_plus_inspector/pubspec.yamlpackages/isar_plus_test/test/collection_size_test.dartpackages/isar_plus_test/test/embedded_test.dartpackages/isar_plus_test/test/enum_test.dartpackages/isar_plus_test/test/filter/filter_int_list_test.dartpackages/isar_plus_test/test/inheritance_test.dartpackages/isar_plus_test/test/isolate_test_stub.dartpackages/isar_plus_test/test/query/embedded_test.dartpackages/isar_plus_test/tool/generate_all_tests.dartpackages/isar_plus_test/tool/generate_long_double_test.dart
| final results = isar2.enumModels.where().findAll(); | ||
| expect(results.length, 2); | ||
|
|
||
| // Known value should round-trip correctly | ||
| // ByteEnum2.option1 value = 2 → ByteEnum.option2 (value = 2) | ||
| expect(results[0].byteEnum, ByteEnum.option2); | ||
|
|
||
| // Unknown value (8) should fall back to first enum value | ||
| expect(results[1].byteEnum, ByteEnum.option1); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Isar / isar_plus Dart, is the order of collection.where().findAll() guaranteed when no explicit sort is specified?
💡 Result:
No—when you call collection.where(...).findAll() without specifying an explicit sort, the order of the returned list is not something you can rely on as a stable/guaranteed “no-sort” ordering. What Isar guarantees is only the execution pipeline: it traverses an index to find matching objects, then filters, then (separately) sorts results, and finally applies offset/limit before returning them [1]. If you don’t provide a sort, you shouldn’t assume a specific ordering of the final result list. Practical takeaway: if you need deterministic ordering, always specify a sort (e.g., via .anyId()/sort or other explicit sort options supported by your query) rather than depending on whatever traversal or internal default order might happen to occur in a given version/platform [2][1].
Citations:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/isar_plus_test/test/enum_test.dart"
# Show the relevant section around the reported lines
nl -ba "$FILE" | sed -n '340,430p'
# Find where enumModels rows are written/created (helps confirm ids)
rg -n "enumModels\.(put|putAll|add|addAll|getAll|get|delete|where\(|anyId|sort|findAll)" "$FILE"
# Also look for migrations/setup that create the two records
rg -n "EnumModel|ByteEnum2|ByteEnum|migrat|schema|isar2" "$FILE" | head -n 200Repository: ahmtydn/isar_plus
Length of output: 104
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/isar_plus_test/test/enum_test.dart"
# Print the reported region with line numbers (no nl dependency)
cat -n "$FILE" | sed -n '350,450p'
# Find usage that would clarify record ids and ordering for enumModels
rg -n "enumModels\.(put|putAll|add|addAll|getAll|get|delete|where\(|anyId|sort|findAll)" "$FILE" || true
# Find where EnumModel / ByteEnum2 / ByteEnum are set up or migrated
rg -n "EnumModel|ByteEnum2|ByteEnum|migrat|schema|isar2" "$FILE" | head -n 200 || trueRepository: ahmtydn/isar_plus
Length of output: 5604
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/isar_plus_test/test/enum_test.dart"
# Inspect model definitions around EnumModel / EnumModelV2 (to see `@Id` fields and constructor order)
cat -n "$FILE" | sed -n '1,220p'
# Inspect the earlier migration test setup (around where isar1/schema evolve)
cat -n "$FILE" | sed -n '220,380p'
# Specifically inspect around the "Removed value" block to see if ids 1/2 are preserved
cat -n "$FILE" | sed -n '380,430p'Repository: ahmtydn/isar_plus
Length of output: 13539
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/isar_plus_test/test/enum_test.g.dart"
# If g.dart isn't in repo, fall back to searching under test dir
if [ ! -f "$FILE" ]; then
FILE="$(fd -a "enum_test.g.dart" packages/isar_plus_test/test -t f | head -n 1 || true)"
fi
echo "Using: $FILE"
rg -n "enumModels|EnumModel|`@Id`|Id|get\\(|isarModels|Schema" "$FILE" | head -n 80Repository: ahmtydn/isar_plus
Length of output: 128
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "EnumModelSchema|EnumModelV2Schema" -S .Repository: ahmtydn/isar_plus
Length of output: 719
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find any generated files for enum_test
fd -a "enum_test.g.dart" . -t f || true
fd -a "*EnumModel*Schema*.dart" . -t f || true
fd -a "*enum*schema*.dart" packages/isar_plus_test/test -t f || true
# Search for the schema symbol definitions
rg -n "EnumModelSchema|EnumModelV2Schema" -S packages/isar_plus_test .
rg -n "class EnumModelSchema|class EnumModelV2Schema|EnumModelSchema\\s*=" -S . || trueRepository: ahmtydn/isar_plus
Length of output: 1984
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/isar_plus_test/test/enum_test.dart"
cat -n "$FILE" | sed -n '360,410p'Repository: ahmtydn/isar_plus
Length of output: 1776
Avoid relying on findAll() order for migrated-row assertions.
where().findAll() without an explicit sort doesn’t guarantee stable ordering, so results[0] / results[1] can flip. Since the migrated records have explicit ids (1 and 2), assert via .get() instead.
Suggested patch
- final results = isar2.enumModels.where().findAll();
- expect(results.length, 2);
+ final results = isar2.enumModels.where().findAll();
+ expect(results.length, 2);
+ final row1 = isar2.enumModels.get(1)!;
+ final row2 = isar2.enumModels.get(2)!;
// Known value should round-trip correctly
// ByteEnum2.option1 value = 2 → ByteEnum.option2 (value = 2)
- expect(results[0].byteEnum, ByteEnum.option2);
+ expect(row1.byteEnum, ByteEnum.option2);
// Unknown value (8) should fall back to first enum value
- expect(results[1].byteEnum, ByteEnum.option1);
+ expect(row2.byteEnum, ByteEnum.option1);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| final results = isar2.enumModels.where().findAll(); | |
| expect(results.length, 2); | |
| // Known value should round-trip correctly | |
| // ByteEnum2.option1 value = 2 → ByteEnum.option2 (value = 2) | |
| expect(results[0].byteEnum, ByteEnum.option2); | |
| // Unknown value (8) should fall back to first enum value | |
| expect(results[1].byteEnum, ByteEnum.option1); | |
| final results = isar2.enumModels.where().findAll(); | |
| expect(results.length, 2); | |
| final row1 = isar2.enumModels.get(1)!; | |
| final row2 = isar2.enumModels.get(2)!; | |
| // Known value should round-trip correctly | |
| // ByteEnum2.option1 value = 2 → ByteEnum.option2 (value = 2) | |
| expect(row1.byteEnum, ByteEnum.option2); | |
| // Unknown value (8) should fall back to first enum value | |
| expect(row2.byteEnum, ByteEnum.option1); |
🤖 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 `@packages/isar_plus_test/test/enum_test.dart` around lines 393 - 401, The test
currently relies on the unstable order returned by
isar2.enumModels.where().findAll() and indexes results[0]/results[1]; change the
assertions to fetch by id instead (use isar2.enumModels.get(1) and
isar2.enumModels.get(2)) and assert their byteEnum values directly so the
migrated-row checks are stable; update the two expect calls that reference
results[...] to use the corresponding get(id) values and keep the same expected
enums (ByteEnum.option2 for id 1 and ByteEnum.option1 for id 2).
2 participants
Summary by CodeRabbit
Tests
Chores