test: Add contract tests for Log/LogWriter#122
Merged
Conversation
|
Complex PR? Review this PR in Change Stack to move by importance, not file order. Warning Review limit reached
More reviews will be available in 46 minutes and 13 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR introduces ChangesIsolatedObject Package and Integration
🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
nielsenko
force-pushed
the
test-log-contract
branch
6 times, most recently
from
b1f9c0f to
2360dc9
Compare
Crazelu
approved these changes
Jun 5, 2026
nielsenko
force-pushed
the
test-log-contract
branch
from
2360dc9 to
33ebf43
Compare
marcelomendoncasoares
requested changes
Jun 5, 2026
Contributor
marcelomendoncasoares
left a comment
marcelomendoncasoares
left a comment
There was a problem hiding this comment.
Just a comment about the shape of some tests.
- progress_nesting_test: a log emitted inside a progress runner must be attributed to the progress scope via the Zone, and a nested progress must child under the outer scope. - progress_relabel_test: progress must never surface the runner's result as a scope label (uses a generic recording writer, no terminal rendering).
These fail to compile if the runner is narrowed to Future<T> Function()
nielsenko
force-pushed
the
test-log-contract
branch
from
33ebf43 to
d45c27e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/serverpod_logging/test/log_scope_test.dart (1)
5-16: ⚡ Quick winConsider using setUp for shared test fixtures.
For consistency with the other test files in this PR (progress_nesting_test.dart, progress_relabel_test.dart, progress_sync_runner_test.dart), consider extracting the
writerandloginitialization to asetUpblock. Both tests in this group create identical fixtures.♻️ Proposed refactor
group('Given Log scoping', () { + late TestLogWriter writer; + late Log log; + + setUp(() { + writer = TestLogWriter(); + log = Log(writer); + }); + test( 'when a log is emitted outside any progress scope, ' 'then it attaches to the synthetic root scope', () async { - final writer = TestLogWriter(); - final log = Log(writer); - log.info('orphan');🤖 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/serverpod_logging/test/log_scope_test.dart` around lines 5 - 16, Extract the repeated test fixtures into a setUp block: move the TestLogWriter() and Log(...) initialization currently created inside tests of the 'Given Log scoping' group into a group-level setUp that assigns them to variables accessible by the tests, so both tests reuse the same writer and log instances; update tests to use those shared variables (referencing TestLogWriter and Log) and ensure any needed cleanup (e.g., await log.flush()) remains in each test.packages/isolated_object/test/create_test.dart (1)
43-46: ⚡ Quick winTighten the error type assertion.
The test verifies factory failures but uses
throwsA(anything), which accepts any exception. For consistency withevaluate_test.dart(line 112) and to verify the protocol correctly propagates factory errors, assert that the error is aRemoteError.✨ Suggested improvement
await expectLater( isolated.evaluate((counter) => counter.value), - throwsA(anything), + throwsA(isA<RemoteError>()), );🤖 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/isolated_object/test/create_test.dart` around lines 43 - 46, The test currently asserts any exception via throwsA(anything) for the isolated.evaluate((counter) => counter.value) call; change the assertion to expect a RemoteError (e.g. throwsA(isA<RemoteError>())) so the test mirrors evaluate_test.dart and ensures factory errors are propagated as RemoteError; update the expectLater invocation around isolated.evaluate to use throwsA(isA<RemoteError>()).
🤖 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.
Nitpick comments:
In `@packages/isolated_object/test/create_test.dart`:
- Around line 43-46: The test currently asserts any exception via
throwsA(anything) for the isolated.evaluate((counter) => counter.value) call;
change the assertion to expect a RemoteError (e.g. throwsA(isA<RemoteError>()))
so the test mirrors evaluate_test.dart and ensures factory errors are propagated
as RemoteError; update the expectLater invocation around isolated.evaluate to
use throwsA(isA<RemoteError>()).
In `@packages/serverpod_logging/test/log_scope_test.dart`:
- Around line 5-16: Extract the repeated test fixtures into a setUp block: move
the TestLogWriter() and Log(...) initialization currently created inside tests
of the 'Given Log scoping' group into a group-level setUp that assigns them to
variables accessible by the tests, so both tests reuse the same writer and log
instances; update tests to use those shared variables (referencing TestLogWriter
and Log) and ensure any needed cleanup (e.g., await log.flush()) remains in each
test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4a7df97c-361a-4005-8231-3e51ac9f06f2
📒 Files selected for processing (20)
.github/workflows/ci.yml.github/workflows/publish-isolated_object.yamlpackages/isolated_object/CHANGELOG.mdpackages/isolated_object/LICENSEpackages/isolated_object/README.mdpackages/isolated_object/analysis_options.yamlpackages/isolated_object/lib/isolated_object.dartpackages/isolated_object/lib/src/isolated_object.dartpackages/isolated_object/pubspec.yamlpackages/isolated_object/test/close_test.dartpackages/isolated_object/test/create_test.dartpackages/isolated_object/test/evaluate_test.dartpackages/isolated_object/test/lifecycle_test.dartpackages/serverpod_logging/pubspec.yamlpackages/serverpod_logging/test/log_dispatch_test.dartpackages/serverpod_logging/test/log_scope_test.dartpackages/serverpod_logging/test/progress_nesting_test.dartpackages/serverpod_logging/test/progress_relabel_test.dartpackages/serverpod_logging/test/progress_sync_runner_test.dartpubspec.yaml
nielsenko
force-pushed
the
test-log-contract
branch
from
d45c27e to
1d54b4c
Compare
Framework-level tests (no CLI rendering): - Level gating skips the entry factory below the threshold - Runtime logLevel changes take effect; isDebugEnabled tracks it - Writer errors are swallowed (dispatch is best-effort) - Writes serialize in invocation order even when earlier ones are slower - close() drops any further dispatches - Convenience methods map to the right level; error() attaches error + stack trace - Logs emitted outside any progress attach to the synthetic root scope - progress passes metadata through to the opened scope - LogScope parent/child: root has no parent; a child keeps its own id/label/metadata
nielsenko
force-pushed
the
test-log-contract
branch
from
1d54b4c to
b9138c6
Compare
marcelomendoncasoares
approved these changes
Jun 5, 2026
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
3 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.
Framework-level tests pinning the Log contract
Summary by CodeRabbit
New Features
isolated_objectpackage to wrap objects in dedicated isolates, enabling concurrent operation execution and keeping animations/event-loops responsive during blocking synchronous work.Tests
Chores