Capture stderr in regression case logs - #3471
RanaPriyansh wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
There are correctness and consistency issues in the updated command construction and CTest properties that should be addressed before merging.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (2)
What changed in this PR
This PR updates the regression-test Python driver helper to ensure stderr is captured in per-case log files for non-verbose runs, addressing Issue #3448 where failures could appear “silent” under CTest defaults. It also adds focused Python/CTest coverage to prevent regressions while preserving verbose behavior and child exit codes.
Changes:
- Merge stderr into the per-case
.logoutput for normal and restart driver invocations. - Add a Python unittest suite that verifies stdout/stderr capture behavior, log overwrite semantics, and exit-code preservation.
- Register the new unittest under CTest with labels intended for Python-focused CI selection.
| File | Description |
|---|---|
reg_tests/lib/openfastDrivers.py |
Updates command redirection so non-verbose case logs include stderr. |
reg_tests/lib/test_openfastDrivers.py |
Adds unit tests validating stderr capture and verbose/non-verbose stream behavior. |
reg_tests/CTestList.cmake |
Adds a new CTest entry to run the Python unittest in CI. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| add_test(openfast_driver_stderr "${Python_EXECUTABLE}" "${CMAKE_CURRENT_LIST_DIR}/lib/test_openfastDrivers.py" -v) | ||
| set_tests_properties(openfast_driver_stderr PROPERTIES LABELS "python;driver") |
There was a problem hiding this comment.
Added TIMEOUT 120 to the openfast_driver_stderr CTest properties. The test resolves its source and child paths explicitly, so it does not need a WORKING_DIRECTORY override. The real-file CMake harness reports the expected command, python;driver labels, and TIMEOUT 120.
| elif restart: | ||
| command = f"{executable} -restart {os.path.splitext(inputFile)[0]} > {logFile}" | ||
| command = f"{executable} -restart {os.path.splitext(inputFile)[0]} > {logFile} 2>&1" | ||
| else: | ||
| command = f"{executable} {inputFile} {ExtraFlags} > {logFile}" | ||
| command = f"{executable} {inputFile} {ExtraFlags} > {logFile} 2>&1" |
There was a problem hiding this comment.
This is a valid pre-existing issue, but it is outside the stderr-capture scope of this PR. The follow-up preserves the existing command construction and adds no path-handling behavior. A separate issue should track shell quoting for paths that contain spaces.
Co-authored-by: OpenAI ChatGPT <chatgpt@openai.com>
Feature or improvement description
Capture stderr in non-verbose regression case logs for normal and restart commands. Preserve verbose output and child exit status.
Related issue, if one exists
Fixes #3448.
Impacted areas of the software
reg_tests/lib/openfastDrivers.pyand focused Python/CTest coverage.Additional supporting information
The test commit precedes the fix commit. The new CTest entry uses the existing interface CI selection and has a 120-second timeout.
Generative AI usage
OpenAI models generated the patch and tests and assisted analysis and review.
Co-authored-by: OpenAI ChatGPT chatgpt@openai.com
Test results, if applicable
9d94ca8: four subprocess cases fail because stderr is absent from the log. Separate preservation and verbose controls pass.d837f3f: all five focused unittest methods pass. The complete contribution passesgit diff --check.openfast_driver_stderrwith the expected command,python;driverlabels, andTIMEOUT 120. The focused CTest test passes.d837f3f. CI for the timeout follow-up is pending.This PR remains a draft pending required automated checks.