feat: Add serverpod_logging_cli bridge package#124
Open
nielsenko wants to merge 5 commits into
Open
Conversation
|
Too many files changed? Review this PR in Change Stack to see how the pieces fit before you dive in. Warning Review limit reached
More reviews will be available in 27 minutes and 21 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 (21)
📝 WalkthroughWalkthroughThis PR introduces two new Dart packages— ChangesIsolated Object Package
Logging Infrastructure Extensions
Workspace and CI Automation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
feat-serverpod-cli-logger
branch
6 times, most recently
from
cfdb63e to
980eb75
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
packages/serverpod_logging/test/isolated_log_writer_test.dart (2)
72-72: ⚡ Quick winTest name should include the "when" clause.
Per coding guidelines, test names should follow the "when … then" pattern. This test describes the outcome but omits the action/trigger.
Suggested naming improvement
test( 'when openScope and closeScope are called, then they are forwarded', () async {As per coding guidelines, use the Given-when-then pattern: start test names with "when" to describe the action, followed by "then" for the expected outcome.
🤖 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/isolated_log_writer_test.dart` at line 72, Rename the test named 'then openScope and closeScope are forwarded' in isolated_log_writer_test.dart to follow the Given-when-then pattern; update the test() call's description string to something like 'when openScope and closeScope are called, then they are forwarded' so the name includes the "when" clause and clearly describes the action and expected outcome for the test that asserts forwarding of openScope/closeScope.
55-56: ⚡ Quick winTest name should include the "when" clause.
Per coding guidelines, test names should follow the "when … then" pattern. This test describes the outcome ("then log is forwarded") but omits the action/trigger ("when log is called").
Suggested naming improvement
test( 'when log is called, then it is forwarded to the writer running in the isolate', () async {As per coding guidelines, use the Given-when-then pattern: start test names with "when" to describe the action, followed by "then" for the expected outcome.
🤖 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/isolated_log_writer_test.dart` around lines 55 - 56, Rename the test title in isolated_log_writer_test.dart for the test invocation currently starting with 'then log is forwarded to the writer running in the isolate' to follow the Given-when-then pattern by adding the "when" clause (e.g., change to "when log is called, then it is forwarded to the writer running in the isolate"); update the string passed to the test(...) call so the test name begins with "when" and retains the same assertion logic in the test body.packages/serverpod_logging/lib/src/isolated_log_writer.dart (1)
25-41: ⚡ Quick winConsider adding test coverage for
stackTraceparameter.The
closeScopeimplementation forwards the optionalstackTraceparameter across the isolate boundary. While the class documentation correctly warns that "payloads must be sendable" (line 8), the current test suite doesn't exercise the case where astackTraceis provided. Adding a test case would increase confidence that typicalStackTraceimplementations (e.g., fromStackTrace.current) are reliably sendable.🧪 Example test case to add
test('when closeScope includes a stackTrace, then it is forwarded', () async { final writer = IsolatedLogWriter(() => TestLogWriter()); final scope = LogScope.root('op'); final st = StackTrace.current; await writer.openScope(scope); await writer.closeScope( scope, success: false, duration: Duration.zero, error: StateError('test'), stackTrace: st, ); // Verify the wrapped writer received the stackTrace final closedScopes = await writer.evaluate( (w) => (w as TestLogWriter).closedScopes, ); expect(closedScopes, hasLength(1)); await writer.close(); });🤖 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/lib/src/isolated_log_writer.dart` around lines 25 - 41, Add a unit test that constructs an IsolatedLogWriter (e.g., IsolatedLogWriter(() => TestLogWriter())), opens a LogScope (LogScope.root('op')), captures a StackTrace (StackTrace.current), calls closeScope(scope, success: false, duration: Duration.zero, error: StateError('test'), stackTrace: st) and then uses writer.evaluate((w) => (w as TestLogWriter).closedScopes) to assert the wrapped TestLogWriter received one closed scope containing the same stackTrace; this ensures closeScope (the method that forwards error and stackTrace across the isolate via evaluate/closeScope) properly transmits StackTrace values.packages/isolated_object/test/close_test.dart (1)
11-21: 💤 Low valueTest name "then it handles it gracefully" is vague.
The guideline states: "Describe behaviour, not implementation, in test names." The test verifies two specific behaviors: (1) calling
close()multiple times does not throw, and (2)isClosedis true. Consider a more precise description such as"then it is idempotent and isClosed remains true"or"then subsequent close calls do not throw".As per coding guidelines: "Describe behaviour, not implementation, in test names (e.g., 'then progress is marked failed' not 'then fail() is called')."
🤖 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/close_test.dart` around lines 11 - 21, Rename the vague test description to explicitly state the expected behaviors being verified: that calling IsolatedObject.close() multiple times does not throw and that IsolatedObject.isClosed is true; update the test's name string (the test(...) invocation in close_test.dart) to something like "then subsequent close calls do not throw and isClosed remains true" or split into two focused tests named "then subsequent close calls do not throw" and "then isClosed becomes true" so the behavior (idempotence of close and isClosed state) is clearly described.packages/isolated_object/test/lifecycle_test.dart (2)
8-16: 💤 Low valueClarify test name to describe the full state transition.
The test name
'then isClosed is false until close is called'only describes the initial state and omits the transition. The test verifies both pre-close and post-close state with twoexpectcalls, which represents a single behavioral contract (the state transition).Consider renaming to describe the complete behavior:
📝 Suggested test name
test( - 'Given an open IsolatedObject, ' - 'then isClosed is false until close is called', () async { + 'Given an IsolatedObject, ' + 'when close is called, ' + 'then isClosed transitions from false to true', () async { final isolated = IsolatedObject<_Counter>(() => _Counter(0));As per coding guidelines: "Describe behaviour, not implementation, in test names" and test names should clearly express the when–then relationship.
🤖 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/lifecycle_test.dart` around lines 8 - 16, Rename the test to describe the full when–then state transition: update the test declaration that uses test(...) which constructs an IsolatedObject<_Counter> and asserts isolated.isClosed before and after calling isolated.close(), to a name that expresses both pre-close and post-close behavior (e.g., "when opened, isClosed is false and becomes true after close") so the test describes the complete behavior rather than only the initial state.
7-44: ⚡ Quick winOrganize tests with top-level Given groups.
The coding guidelines require organizing tests with top-level
group('Given …')blocks for preconditions and shared setup. Adding a group would improve consistency with the project's test conventions.📋 Suggested structure
void main() { + group('Given IsolatedObject lifecycle', () { test( 'Given an open IsolatedObject, ' 'then isClosed is false until close is called', () async { // ... existing test body }); test( 'Given a closed IsolatedObject, ' 'when evaluate is called, ' 'then it throws a StateError', () async { // ... existing test body }); test( 'Given keepIsolateAlive: false, ' 'when the object is used, ' 'then it still evaluates correctly', () async { // ... existing test body }); + }); }As per coding guidelines: "Organize tests with top-level group('Given …') for preconditions and shared setup".
🤖 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/lifecycle_test.dart` around lines 7 - 44, Wrap the three standalone tests in a top-level group with a "Given …" description to follow the project's test convention; create a group('Given an IsolatedObject') { ... } that contains the tests that reference IsolatedObject<_Counter>, keeping the existing test bodies (including calls to IsolatedObject, evaluate, close, and keepIsolateAlive) unchanged and only moving them into the group to centralize preconditions and shared setup for these lifecycle tests.packages/isolated_object/test/evaluate_test.dart (1)
9-243: ⚡ Quick winRename test groups to follow Given–when–then pattern.
The coding guidelines require top-level groups to start with
'Given …'to describe preconditions. The current groups ('evaluate','error handling', etc.) describe features rather than preconditions.📋 Suggested group names
- group('evaluate', () { + group('Given an IsolatedObject with initial state', () { // ... existing tests }); - group('error handling', () { + group('Given an IsolatedObject that encounters errors', () { // ... existing tests }); - group('concurrent operations', () { + group('Given an IsolatedObject with concurrent evaluations', () { // ... existing tests }); - group('complex objects', () { + group('Given an IsolatedObject with complex nested state', () { // ... existing tests });As per coding guidelines: "Organize tests with top-level group('Given …') for preconditions and shared setup" and "Start Given group names with 'Given …'".
🤖 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/evaluate_test.dart` around lines 9 - 243, Rename the top-level test group titles to follow the Given–when–then pattern by changing the four group(...) declarations currently named 'evaluate', 'error handling', 'concurrent operations', and 'complex objects' to start with "Given …" and describe the precondition (e.g. "Given an IsolatedObject with initial state" for evaluate, "Given an IsolatedObject when errors occur" for error handling, "Given an IsolatedObject with concurrent requests" for concurrent operations, and "Given an IsolatedObject with complex state" for complex objects) so the tests using setUp/tearDown and functions like IsolatedObject<_Counter>() and IsolatedObject<_ComplexObject>() clearly reflect the shared preconditions.packages/isolated_object/test/create_test.dart (1)
7-48: ⚡ Quick winOrganize tests with top-level Given groups.
The coding guidelines require organizing tests with top-level
group('Given …')blocks for preconditions and shared setup. While these tests have different preconditions, grouping them would improve structure and make it easier to add shared setup later.📋 Suggested structure
void main() { + group('Given IsolatedObject creation', () { test( 'Given a synchronous factory, ' 'when IsolatedObject is created, ' 'then it successfully initializes', () async { // ... existing test body }); test( 'Given an async factory, ' 'when IsolatedObject is created, ' 'then it successfully initializes', () async { // ... existing test body }); test( 'Given a factory that throws, ' 'when IsolatedObject is created, ' 'then subsequent operations fail gracefully', () async { // ... existing test body }); + }); }As per coding guidelines: "Organize tests with top-level group('Given …') for preconditions and shared setup".
🤖 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 7 - 48, Wrap the individual tests inside top-level group('Given …') blocks to reflect preconditions and allow shared setup/teardown: modify main() to create three groups (e.g., group('Given a synchronous factory', ...), group('Given an async factory', ...), group('Given a factory that throws', ...)) and move the corresponding test(...) calls into their respective group bodies, keeping the test names and assertions unchanged; if any shared initialization/cleanup for a group is needed later, add setUp/setUpAll or tearDown/tearDownAll inside the group where IsolatedObject instances are created or closed.
🤖 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/isolated_object/lib/src/isolated_object.dart`:
- Around line 122-135: evaluate has a TOCTOU race: after awaiting _connected it
may still add a completer to inflight even if close() set _isClosed and cleared
inflight; to fix, re-check _isClosed immediately after awaiting _connected
(before touching inflight, incrementing _nextId or calling toChild.send) and
throw the same StateError if closed so no completer is registered or message
sent; update the evaluate(T) method to perform this second check (use the
existing _isClosed and _connected variables and keep inflight and completer
handling unchanged otherwise).
In `@packages/serverpod_logging_cli/test/serverpod_cli_logger_test.dart`:
- Line 166: Rename the test group named "log level handling" to follow the
Given-when-then pattern by starting with "when" (e.g., "when handling log
levels" or "when adjusting log levels") so it matches the nested groups like
"when calling progress" and "when calling progressStream"; update the group(...)
label where it appears in serverpod_cli_logger_test.dart to use the new
lowercase "when …" phrasing.
---
Nitpick comments:
In `@packages/isolated_object/test/close_test.dart`:
- Around line 11-21: Rename the vague test description to explicitly state the
expected behaviors being verified: that calling IsolatedObject.close() multiple
times does not throw and that IsolatedObject.isClosed is true; update the test's
name string (the test(...) invocation in close_test.dart) to something like
"then subsequent close calls do not throw and isClosed remains true" or split
into two focused tests named "then subsequent close calls do not throw" and
"then isClosed becomes true" so the behavior (idempotence of close and isClosed
state) is clearly described.
In `@packages/isolated_object/test/create_test.dart`:
- Around line 7-48: Wrap the individual tests inside top-level group('Given …')
blocks to reflect preconditions and allow shared setup/teardown: modify main()
to create three groups (e.g., group('Given a synchronous factory', ...),
group('Given an async factory', ...), group('Given a factory that throws', ...))
and move the corresponding test(...) calls into their respective group bodies,
keeping the test names and assertions unchanged; if any shared
initialization/cleanup for a group is needed later, add setUp/setUpAll or
tearDown/tearDownAll inside the group where IsolatedObject instances are created
or closed.
In `@packages/isolated_object/test/evaluate_test.dart`:
- Around line 9-243: Rename the top-level test group titles to follow the
Given–when–then pattern by changing the four group(...) declarations currently
named 'evaluate', 'error handling', 'concurrent operations', and 'complex
objects' to start with "Given …" and describe the precondition (e.g. "Given an
IsolatedObject with initial state" for evaluate, "Given an IsolatedObject when
errors occur" for error handling, "Given an IsolatedObject with concurrent
requests" for concurrent operations, and "Given an IsolatedObject with complex
state" for complex objects) so the tests using setUp/tearDown and functions like
IsolatedObject<_Counter>() and IsolatedObject<_ComplexObject>() clearly reflect
the shared preconditions.
In `@packages/isolated_object/test/lifecycle_test.dart`:
- Around line 8-16: Rename the test to describe the full when–then state
transition: update the test declaration that uses test(...) which constructs an
IsolatedObject<_Counter> and asserts isolated.isClosed before and after calling
isolated.close(), to a name that expresses both pre-close and post-close
behavior (e.g., "when opened, isClosed is false and becomes true after close")
so the test describes the complete behavior rather than only the initial state.
- Around line 7-44: Wrap the three standalone tests in a top-level group with a
"Given …" description to follow the project's test convention; create a
group('Given an IsolatedObject') { ... } that contains the tests that reference
IsolatedObject<_Counter>, keeping the existing test bodies (including calls to
IsolatedObject, evaluate, close, and keepIsolateAlive) unchanged and only moving
them into the group to centralize preconditions and shared setup for these
lifecycle tests.
In `@packages/serverpod_logging/lib/src/isolated_log_writer.dart`:
- Around line 25-41: Add a unit test that constructs an IsolatedLogWriter (e.g.,
IsolatedLogWriter(() => TestLogWriter())), opens a LogScope
(LogScope.root('op')), captures a StackTrace (StackTrace.current), calls
closeScope(scope, success: false, duration: Duration.zero, error:
StateError('test'), stackTrace: st) and then uses writer.evaluate((w) => (w as
TestLogWriter).closedScopes) to assert the wrapped TestLogWriter received one
closed scope containing the same stackTrace; this ensures closeScope (the method
that forwards error and stackTrace across the isolate via evaluate/closeScope)
properly transmits StackTrace values.
In `@packages/serverpod_logging/test/isolated_log_writer_test.dart`:
- Line 72: Rename the test named 'then openScope and closeScope are forwarded'
in isolated_log_writer_test.dart to follow the Given-when-then pattern; update
the test() call's description string to something like 'when openScope and
closeScope are called, then they are forwarded' so the name includes the "when"
clause and clearly describes the action and expected outcome for the test that
asserts forwarding of openScope/closeScope.
- Around line 55-56: Rename the test title in isolated_log_writer_test.dart for
the test invocation currently starting with 'then log is forwarded to the writer
running in the isolate' to follow the Given-when-then pattern by adding the
"when" clause (e.g., change to "when log is called, then it is forwarded to the
writer running in the isolate"); update the string passed to the test(...) call
so the test name begins with "when" and retains the same assertion logic in the
test body.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7232dcda-8495-43d9-bc50-5e7443eb945e
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (39)
.github/workflows/ci.yml.github/workflows/publish-isolated_object.yaml.github/workflows/publish-serverpod_logging_cli.yaml.gitignorepackages/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/CHANGELOG.mdpackages/serverpod_logging/lib/serverpod_logging.dartpackages/serverpod_logging/lib/src/isolated_log_writer.dartpackages/serverpod_logging/lib/src/log.dartpackages/serverpod_logging/lib/src/log_types.dartpackages/serverpod_logging/pubspec.yamlpackages/serverpod_logging/test/isolated_log_writer_test.dartpackages/serverpod_logging/test/log_dispatch_test.dartpackages/serverpod_logging/test/log_progress_test.dartpackages/serverpod_logging/test/log_scope_test.dartpackages/serverpod_logging/test/multi_log_writer_test.dartpackages/serverpod_logging/test/progress_nesting_test.dartpackages/serverpod_logging/test/progress_relabel_test.dartpackages/serverpod_logging/test/progress_sync_runner_test.dartpackages/serverpod_logging_cli/CHANGELOG.mdpackages/serverpod_logging_cli/LICENSEpackages/serverpod_logging_cli/README.mdpackages/serverpod_logging_cli/analysis_options.yamlpackages/serverpod_logging_cli/lib/serverpod_logging_cli.dartpackages/serverpod_logging_cli/lib/src/serverpod_cli_logger.dartpackages/serverpod_logging_cli/lib/src/std_out_log_writer.dartpackages/serverpod_logging_cli/pubspec.yamlpackages/serverpod_logging_cli/test/serverpod_cli_logger_test.dartpubspec.yaml
Comment on lines
+122
to
+135
| Future<U> evaluate<U>(FutureOr<U> Function(T) function) async { | ||
| if (_isClosed) { | ||
| throw StateError('IsolatedObject<$T> is closed'); | ||
| } | ||
|
|
||
| final (toChild, _, inflight) = await _connected; | ||
|
|
||
| final id = _nextId++; | ||
| final completer = Completer<U>(); | ||
| inflight[id] = completer; | ||
|
|
||
| toChild.send((id: id, function: function)); | ||
| return completer.future; | ||
| } |
There was a problem hiding this comment.
TOCTOU race: evaluate may add to inflight after close clears it.
There is a time-of-check-time-of-use race between Line 123 (_isClosed check) and Line 131 (inflight assignment). If close() runs after evaluate() checks _isClosed but before evaluate() resumes from await _connected, the following sequence occurs:
evaluate()checks_isClosed(false), then suspends atawait _connected.close()sets_isClosed = true, awaits_connected, and clearsinflight.evaluate()resumes and adds its completer to the now-emptyinflightmap, then sends the action.
The response may arrive with no registered completer (triggering the assertion at Line 62 in debug mode, or silently returning in release). This violates fail-fast semantics—evaluate() should throw immediately when the object is closed.
🔒 Proposed fix: re-check _isClosed after await
Future<U> evaluate<U>(FutureOr<U> Function(T) function) async {
if (_isClosed) {
throw StateError('IsolatedObject<$T> is closed');
}
final (toChild, _, inflight) = await _connected;
+ if (_isClosed) {
+ throw StateError('IsolatedObject<$T> is closed');
+ }
+
final id = _nextId++;
final completer = Completer<U>();
inflight[id] = completer;
toChild.send((id: id, function: function));
return completer.future;
}📝 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.
Suggested change
| Future<U> evaluate<U>(FutureOr<U> Function(T) function) async { | |
| if (_isClosed) { | |
| throw StateError('IsolatedObject<$T> is closed'); | |
| } | |
| final (toChild, _, inflight) = await _connected; | |
| final id = _nextId++; | |
| final completer = Completer<U>(); | |
| inflight[id] = completer; | |
| toChild.send((id: id, function: function)); | |
| return completer.future; | |
| } | |
| Future<U> evaluate<U>(FutureOr<U> Function(T) function) async { | |
| if (_isClosed) { | |
| throw StateError('IsolatedObject<$T> is closed'); | |
| } | |
| final (toChild, _, inflight) = await _connected; | |
| if (_isClosed) { | |
| throw StateError('IsolatedObject<$T> is closed'); | |
| } | |
| final id = _nextId++; | |
| final completer = Completer<U>(); | |
| inflight[id] = completer; | |
| toChild.send((id: id, function: function)); | |
| return completer.future; | |
| } |
🤖 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/lib/src/isolated_object.dart` around lines 122 -
135, evaluate has a TOCTOU race: after awaiting _connected it may still add a
completer to inflight even if close() set _isClosed and cleared inflight; to
fix, re-check _isClosed immediately after awaiting _connected (before touching
inflight, incrementing _nextId or calling toChild.send) and throw the same
StateError if closed so no completer is registered or message sent; update the
evaluate(T) method to perform this second check (use the existing _isClosed and
_connected variables and keep inflight and completer handling unchanged
otherwise).
980eb75 to
e035dff
Compare
nielsenko
force-pushed
the
feat-serverpod-cli-logger
branch
from
e035dff to
11d1ccb
Compare
- 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
feat-serverpod-cli-logger
branch
from
11d1ccb to
ed9e6f2
Compare
marcelomendoncasoares
requested changes
Jun 5, 2026
Contributor
marcelomendoncasoares
left a comment
marcelomendoncasoares
left a comment
There was a problem hiding this comment.
One nit on the YAML format and some suggestions for improvement on the tests.
Comment on lines
+18
to
+25
| package: | ||
| [ | ||
| cli_tools, | ||
| config, | ||
| isolated_object, | ||
| serverpod_logging, | ||
| serverpod_logging_cli, | ||
| ] |
Contributor
There was a problem hiding this comment.
Suggestion(NIT): Use the more idiomatic YAML format for arrays.
Suggested change
| package: | |
| [ | |
| cli_tools, | |
| config, | |
| isolated_object, | |
| serverpod_logging, | |
| serverpod_logging_cli, | |
| ] | |
| package: | |
| - cli_tools | |
| - config | |
| - isolated_object | |
| - serverpod_logging | |
| - serverpod_logging_cli |
Comment on lines
+23
to
+41
| group('when calling progress (non-stream)', () { | ||
| test( | ||
| 'then it opens and closes exactly one scope with the runner verdict', | ||
| () async { | ||
| final result = await logger.progress('op', () async => true); | ||
|
|
||
| expect(result, isTrue); | ||
| expect(writer.openedScopes.map((s) => s.label), ['op']); | ||
| expect(writer.closedScopes.single.success, isTrue); | ||
| }, | ||
| ); | ||
|
|
||
| test('then a false runner closes the scope unsuccessfully', () async { | ||
| final result = await logger.progress('op', () async => false); | ||
|
|
||
| expect(result, isFalse); | ||
| expect(writer.closedScopes.single.success, isFalse); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
Suggestion: Remove the group and move the "when" to each test description detailing the different parameter on each call.
Comment on lines
+43
to
+52
| group('when calling progressStream', () { | ||
| test( | ||
| 'then each event becomes a sub-operation nested under the parent ' | ||
| '(the "db shape": one parent span, one child span per event)', | ||
| () async { | ||
| final result = await logger.progressStream( | ||
| 'Deploying', | ||
| Stream.fromIterable(['build', 'deploy']), | ||
| toMessage: (s) => s, | ||
| ); |
Contributor
There was a problem hiding this comment.
Suggestion: Same for these tests. Each one of them call progressStream with different parameters, and this means that they represent different actions.
Comment on lines
+166
to
+216
| group('log level handling', () { | ||
| test('a log entry carries its cli LogType in metadata', () async { | ||
| logger.info('hello', type: cli.TextLogType.bullet); | ||
| await logger.flush(); | ||
|
|
||
| final entry = writer.entries.single; | ||
| expect(entry.message, 'hello'); | ||
| expect(entry.metadata?[logTypeKey], cli.TextLogType.bullet); | ||
| }); | ||
|
|
||
| test( | ||
| 'log/write with cli.LogLevel.nothing is suppressed, not an error', | ||
| () async { | ||
| logger.log('x', cli.LogLevel.nothing); | ||
| logger.write('y', cli.LogLevel.nothing); | ||
| await logger.flush(); | ||
|
|
||
| expect(writer.entries, isEmpty); | ||
| }, | ||
| ); | ||
|
|
||
| test( | ||
| 'an error log keeps the stack trace structured on the entry', | ||
| () async { | ||
| final st = StackTrace.current; | ||
| logger.error('bad', stackTrace: st); | ||
| await logger.flush(); | ||
|
|
||
| final entry = writer.entries.single; | ||
| expect(entry.level, LogLevel.error); | ||
| expect(entry.message, 'bad', reason: 'message is not flattened'); | ||
| expect(entry.stackTrace, same(st)); | ||
| }, | ||
| ); | ||
|
|
||
| test( | ||
| 'when silenced (cli.LogLevel.nothing) nothing is logged or scoped', | ||
| () async { | ||
| logger.logLevel = cli.LogLevel.nothing; | ||
|
|
||
| logger.info('dropped'); | ||
| final result = await logger.progress('op', () async => true); | ||
| await logger.flush(); | ||
|
|
||
| expect(result, isTrue, reason: 'runner still runs while silent'); | ||
| expect(writer.entries, isEmpty); | ||
| expect(writer.openedScopes, isEmpty); | ||
| }, | ||
| ); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
Suggestion: These tests are out of the pattern.
nielsenko
force-pushed
the
feat-serverpod-cli-logger
branch
from
ed9e6f2 to
e8f5c21
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
feat-serverpod-cli-logger
branch
from
e8f5c21 to
94b1bbf
Compare
A LogWriter that runs any wrapped writer on a dedicated isolate (via IsolatedObject), so timer-driven spinners keep animating even when the calling isolate is blocked.
A cli_tools.Logger implementation (ServerpodCliLogger) backed by the serverpod_logging LogWriter architecture, plus StdOutLogWriter (a TextLogWriter that renders LogType-aware output via cli_tools' StdOutLogger).
nielsenko
force-pushed
the
feat-serverpod-cli-logger
branch
from
94b1bbf to
c223e7b
Compare
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.
New
serverpod_logging_clibridge package.Includes:
ServerpodCliLogger(acli_tools.Loggerbuilt onLog/LogWriter)StdOutLogWriter(aLogWriterbuild oncli_Tools.StdOutLogger),IsolatedLogWriter