No prints to stdout when using --report-json#108
Conversation
📝 WalkthroughWalkthroughLogging and stdout handling were changed so that when Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI
participant Options
participant Logger
participant App
participant Stdout
User->>CLI: invoke with --report-json
CLI->>Options: parse args
Options->>Logger: disable()
App->>Logger: emit report-level logs (suppressed)
App->>Stdout: write JSON via to_stdout() (or write to file)
Stdout-->>User: only JSON printed to stdout
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
✏️ Tip: You can disable this entire section by setting 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 |
![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
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Line 11: Fix the typo in the CHANGELOG entry by replacing "Cuucmber" with
"Cucumber" in the line that reads "Removed Cuucmber prints to `stdout` when
using `--report-json`..." so the sentence reads "Removed Cucumber prints to
`stdout` when using `--report-json`..." and keep the rest of the text unchanged.
🧹 Nitpick comments (3)
gtest/CMakeLists.txt (2)
4-4: Consider addingREQUIREDor handling missing dependency gracefully.If
nlohmann_jsonis not found, the build will fail with a confusing linker error. Either mark it asREQUIREDto get a clear error message, or wrap the JSON-dependent functionality in a conditional.💡 Suggested fix
-find_package(nlohmann_json) +find_package(nlohmann_json REQUIRED)
45-45:add_compile_definitionswithout target scope affects all targets.
add_compile_definitions(WITH_JSON)applies globally to all targets in this directory and below. Usetarget_compile_definitionsto scope it to the test target only.💡 Suggested fix
target_link_libraries(${target} cucumber-no-main GTest::gtest_main nlohmann_json::nlohmann_json) - add_compile_definitions(WITH_JSON) + target_compile_definitions(${target} PRIVATE WITH_JSON)gtest/stdout_print.cc (1)
6-9: Inconsistent include path forreport.hpp.The include at line 9 uses
"report.hpp"while lines 6-7 use"../src/log.hpp"and"../src/options.hpp". For consistency with the rest of the file, consider using the full relative path.♻️ Suggested fix
`#include` "../src/log.hpp" `#include` "../src/options.hpp" -#include "report.hpp" +#include "../src/report.hpp" `#include` "test_paths.hpp"
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
CHANGELOG.mdgtest/CMakeLists.txtgtest/stdout_print.ccsrc/cucumber.cppsrc/log.hppsrc/options.cppsrc/util.hpp
🧰 Additional context used
🧬 Code graph analysis (5)
src/util.hpp (1)
src/log.hpp (2)
to_stdout(144-147)to_stdout(144-144)
src/log.hpp (1)
gtest/stdout_print.cc (1)
args(19-24)
src/options.cpp (1)
src/log.hpp (2)
disable(107-107)disable(107-107)
src/cucumber.cpp (2)
src/log.hpp (10)
report(66-69)report(66-66)report(118-121)report(118-118)red(164-167)red(164-164)black(172-175)black(172-172)reset(152-155)reset(152-152)src/test_results.cpp (4)
scenarios_to_string(107-154)scenarios_to_string(107-107)steps_to_string(156-216)steps_to_string(156-156)
gtest/stdout_print.cc (5)
src/options.cpp (2)
get_program_args(252-261)get_program_args(252-253)src/options.hpp (2)
get_program_args(168-169)argc(145-145)src/log.hpp (6)
enable(47-47)enable(47-47)enable(108-108)enable(108-108)disable(107-107)disable(107-107)gtest/run_scenarios_continue.cc (1)
maybe_unused(39-44)src/report.cpp (2)
print_json_to_sink(142-146)print_json_to_sink(142-142)
🪛 LanguageTool
CHANGELOG.md
[grammar] ~11-~11: Ensure spelling is correct
Context: ...r/pull/107))
Fixed
- Removed Cuucmber prints to
stdoutwhen using `--report...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Linux (clang-17, clang++-17, -pr clang)
- GitHub Check: Windows
- GitHub Check: Mac
- GitHub Check: Linux (gcc, g++)
🔇 Additional comments (9)
src/log.hpp (2)
44-45: Thread safety consideration form_disabledflag.If the logger is accessed from multiple threads, reads/writes to
m_disabledcould race. For this use case (set once at startup before concurrent logging), it's likely fine, but consider usingstd::atomic<bool>if thread safety is a concern.Also applies to: 103-103
141-147: Clean design for bypassing logger settings.The
to_stdout()function correctly writes directly to stdout without checking the disabled state, which is exactly what's needed for JSON output when logging is disabled. This enables the PR goal of clean JSON piping.src/util.hpp (1)
30-33: Correct use ofto_stdout()for stdout output path.This change ensures that when outputting to stdout (empty filepath), the data bypasses the logger's disabled state, enabling JSON output to work correctly when
--report-jsonis used without a file path.src/options.cpp (1)
137-148: Logic for--report-jsondisabling logs looks correct.When
--report-jsonis set, logging is disabled before any level adjustments. This means--quietor--verboseflags have no effect when--report-jsonis active, which aligns with the PR goal of producing only JSON output.src/cucumber.cpp (2)
27-35: LGTM!The migration from
log::alwaystolog::reportis consistent with the PR objectives. The separate calls maintain the existing pattern and allow the logger's disabled state to suppress all output when--report-jsonis active.
68-80: LGTM!The branching logic correctly separates JSON output (via
report::print_json_to_sink()) from human-readable output (vialog::report). When--report-jsonis set, only JSON is produced; otherwise, the failed scenarios and summary statistics are printed.gtest/stdout_print.cc (3)
19-24: Good addition to restore logging state in TearDown.Calling
cuke::log::enable()ensures tests that disable logging don't affect subsequent tests. This prevents test pollution and maintains test isolation.
432-449: LGTM!The test correctly verifies that
cuke::log::disable()suppresses all stdout output during test execution. The TearDown will re-enable logging for subsequent tests.
450-488: LGTM!The test verifies two key behaviors:
- When
--report-jsonis set, regular logging output is suppressed (empty stdout during test run)- Calling
print_json_to_sink()produces valid, parseable JSONThe approach of capturing stdout twice (once for the test run, once for JSON output) cleanly validates both requirements.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
1 participant
In order to pipe the json output, disable prints to stdout when using
--report-json. Only the json result should be on stdout. For simplification, when printing the output to a file, no output is on stdout from Cucumber.Summary by CodeRabbit
Bug Fixes
Behavior Changes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.