fix(cli): use github reporter in github actions env#9120
Conversation
🦋 Changeset detectedLatest commit: 941f0e0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Merging this PR will not alter performance
Comparing Footnotes
|
WalkthroughAdds automatic GitHub reporter selection for Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_cli/src/runner/impls/finalizers/default.rs`:
- Around line 197-226: The JSON branch currently always writes to
console_reporter_writer; change it to detect cli_reporter.is_file_report() and
route output through the file writer when set: construct or obtain the
FileReporterWriter (instead of console_reporter_writer) and use it for
reporter.write(&mut file_writer, &mut buffer) and for emitting the final JSON
string/pretty-formatted output; ensure JsonReporter, JsonReporterVisitor, and
the formatting/print steps write into the FileReporterWriter when
cli_reporter.is_file_report() is true, otherwise keep using
console_reporter_writer as before.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/biome_cli/Cargo.toml (1)
91-91: Centraliseserial_testin workspace dependencies for consistency. Other external dev-deps are managed in[workspace.dependencies]; addingserial_testthere keeps the version pinned in one place.Optional refactor
-serial_test = "3.3.1" +serial_test = { workspace = true }Then add to
Cargo.tomlunder[workspace.dependencies]:serial_test = "3.3.1"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_cli/Cargo.toml` at line 91, Move the serial_test dev-dependency out of the crate-level Cargo.toml and centralize it in the workspace manifest: remove the line `serial_test = "3.3.1"` from the crate's Cargo.toml and add `serial_test = "3.3.1"` under `[workspace.dependencies]` in the workspace Cargo.toml so the version is pinned centrally for all workspace crates; update any crate Cargo.toml dev-deps to reference the workspace dependency if necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_cli/src/runner/impls/finalizers/default.rs`:
- Around line 109-113: The ConsoleReporter instance is being constructed with
diagnostics_payload moved, but its field is defined as a reference (&'a
DiagnosticsPayload); change the construction to pass a reference (borrow)
instead of the owned value. Update the reporter creation where ConsoleReporter {
summary, diagnostics_payload, execution, ... } is built to use
&diagnostics_payload (or appropriate lifetime-bound reference) so the field type
matches ConsoleReporter::diagnostics_payload; ensure any subsequent uses still
respect the borrowed lifetime and adjust variable names if shadowing is needed.
---
Duplicate comments:
In `@crates/biome_cli/src/runner/impls/finalizers/default.rs`:
- Around line 197-226: The JSON branch currently always writes only to
console_reporter_writer so file report destinations are ignored; update the flow
in the CliReporterKind::Json / JsonPretty arm to detect
cli_reporter.is_file_report() and route output through a FileReporterWriter when
a destination is present. Concretely: keep constructing JsonReporter and
JsonReporterVisitor and calling reporter.write(...) as is, but when
cli_reporter.is_file_report() is true create a FileReporterWriter (using the
same working_directory/verbose context), write the reporter output to that
writer (and/or log to console as before), and otherwise continue using
console_reporter_writer; reference JsonReporter, JsonReporterVisitor,
reporter.write, console_reporter_writer, FileReporterWriter, and
cli_reporter.is_file_report() to locate the changes.
---
Nitpick comments:
In `@crates/biome_cli/Cargo.toml`:
- Line 91: Move the serial_test dev-dependency out of the crate-level Cargo.toml
and centralize it in the workspace manifest: remove the line `serial_test =
"3.3.1"` from the crate's Cargo.toml and add `serial_test = "3.3.1"` under
`[workspace.dependencies]` in the workspace Cargo.toml so the version is pinned
centrally for all workspace crates; update any crate Cargo.toml dev-deps to
reference the workspace dependency if necessary.
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/biome_cli/src/runner/impls/finalizers/default.rs`:
- Around line 211-240: The Json/JsonPretty branch always logs to
console_reporter_writer, ignoring any configured destination; change the flow in
the CliReporterKind::Json | CliReporterKind::JsonPretty arm to choose the writer
based on cli_reporter.destination (or the existing destination field) — if a
destination is present, open/create the target file writer (or use the existing
FileReporterWriter type) and pass that writer into reporter.write and subsequent
log output instead of console_reporter_writer; keep using
console_reporter_writer only when destination is None, and ensure both formatted
(JsonPretty) and raw (Json) outputs are written to the selected writer after
building root via JsonReporterVisitor and formatting when needed (symbols to
edit: JsonReporter, JsonReporterVisitor::new, reporter.write(...),
console_reporter_writer, cli_reporter.kind/CliReporterKind::JsonPretty).
Summary
Closes #9109
Completely missed that one! Fortunately, a user filed an issue against the docs, which are our source of truth, so I think it's a valid bug.
Test Plan
I refactored the tests a bit. I moved all the help tests in their test case, with the help of AI.
I added the crate
serial_testserialise all rage tests, because they use the env temporary dir. I preferred to delegate to a third-party crate as it will help us in the future with other crates that need to run serially.As for testing this feature, I temporarily used
temp_varcrate to inject the environment. It worked fine. However, I had to remove it, because other tests might read the leftovers injected by the crate.I believe a good solution is to use dependency injection via a trait, as we do with
FileSystemandConsole, but it's a big refactor, not something we can do now.Docs