Add system test coverage for partitioned-heat-conduction + fix issues #404, #674, #402#734
Conversation
Registers the partitioned-heat-conduction tutorial with the system test framework. Defines two participants (Dirichlet, Neumann) and six cases across three solver adapters: fenics-adapter, nutils-adapter, and openfoam-adapter. Also adds the reference-results/ directory placeholder and a corresponding partitioned_heat_conduction_test suite in tests.yaml covering the three homogeneous solver combinations (FEniCS, Nutils, OpenFOAM). Related to the GSoC 2026 entry test requirement (add a tutorial to the system tests that does not yet have a metadata.yaml).
…ing (precice#404) Previously, a missing reference archive caused a raw Python exception (tarfile.TarError or FileNotFoundError) that was difficult to interpret. Now __unpack_reference_results() detects the missing file before attempting to open it and raises a FileNotFoundError with a clear message pointing the user to generate_reference_results.py. _run_field_compare() catches that error and returns a FieldCompareResult(exit_code=1) with the message in stderr_data so it surfaces cleanly in the final table.
…limit (precice#402) The simulation timeout was previously hardcoded as GLOBAL_TIMEOUT = 900 s in Systemtest.py with no way to change it at runtime. Changes: - Systemtest dataclass gains a 'timeout: int = GLOBAL_TIMEOUT' field; all three process.communicate() calls (build, run, field-compare) now use self.timeout instead of the module-level constant. - systemtests.py gains a --timeout CLI argument (default 900 s) whose value is forwarded to every Systemtest instance. - Also separates the GitHub step summary markdown rendering from the plain-text terminal output and properly escapes pipe characters and other Markdown-special symbols in cell content (fixes precice#674). Usage examples: python systemtests.py --suites=fenics_test --timeout=300 # catch hangs fast python systemtests.py --suites=fenics_test --timeout=3600 # slow machines
There was a problem hiding this comment.
Pull request overview
Adds system-test scaffolding and runner improvements to support running the partitioned-heat-conduction tutorial in the system-tests framework, while improving robustness and CI summary rendering.
Changes:
- Register
partitioned-heat-conductionin the system test metadata and add a newpartitioned_heat_conduction_testsuite with homogeneous solver combinations. - Improve system test reporting by separating terminal output from GitHub Step Summary Markdown and escaping table cell content.
- Improve system test robustness via configurable per-run
--timeoutand a clearer error path when reference results are missing.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
tools/tests/tests.yaml |
Adds partitioned_heat_conduction_test suite and references expected reference-result archives. |
tools/tests/systemtests/Systemtest.py |
Adds Markdown escaping for GHA summary, handles missing reference results, introduces per-test timeout usage. |
tools/tests/systemtests.py |
Adds --timeout CLI flag and passes it into Systemtest. |
partitioned-heat-conduction/reference-results/.gitkeep |
Adds instructions placeholder for reference results location/generation. |
partitioned-heat-conduction/metadata.yaml |
Adds system-test metadata describing participants and cases for the tutorial. |
Comments suppressed due to low confidence (1)
tools/tests/systemtests/Systemtest.py:503
- Timeout/error handling in _build_docker(): in the exception path you call process.communicate(timeout=SHORT_TIMEOUT) before killing the process. If the initial failure was a TimeoutExpired/hang, this can raise again and leave the docker compose process running. Handle subprocess.TimeoutExpired explicitly and ensure the process is killed (ideally the whole process group) in a finally block before attempting a short communicate/drain.
try:
stdout, stderr = process.communicate(timeout=self.timeout)
except KeyboardInterrupt as k:
process.kill()
# process.send_signal(9)
raise KeyboardInterrupt from k
except Exception as e:
logging.critical(
f"systemtest {self} had serious issues building the docker images via the `docker compose build` command. About to kill the docker compose command. Please check the logs! {e}")
process.communicate(timeout=SHORT_TIMEOUT)
process.kill()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| stdout, stderr = process.communicate(timeout=GLOBAL_TIMEOUT) | ||
| stdout, stderr = process.communicate(timeout=self.timeout) | ||
| except KeyboardInterrupt as k: | ||
| process.kill() | ||
| raise KeyboardInterrupt from k |
There was a problem hiding this comment.
The exception handling around this docker compose execution appears broken because the outer error path later in this method calls logging.CRITICAL(...) (CRITICAL is an int constant, not a logger function). That would raise a TypeError and hide the real failure. Please change it to logging.critical(...) (or logging.exception(...)) so errors are reported correctly.
| def display_systemtestresults_as_table(results: List[SystemtestResult]): | ||
| """ | ||
| Prints the result in a nice tabluated way to get an easy overview | ||
| Prints the result in a nice tabluated way to get an easy overview. |
There was a problem hiding this comment.
Docstring typo: "tabluated" should be "tabulated".
| Prints the result in a nice tabluated way to get an easy overview. | |
| Prints the result in a nice tabulated way to get an easy overview. |
| with tarfile.open(self.reference_result.path) as reference_results_tared: | ||
| # specify which folder to extract to | ||
| reference_results_tared.extractall(self.system_test_dir / PRECICE_REL_REFERENCE_DIR) |
There was a problem hiding this comment.
Using tarfile.extractall() on an archive without validating member paths can allow path traversal (e.g., entries with "../") and overwrite files outside the target directory. Please implement a safe extraction that rejects absolute paths and parent-directory traversals before extracting.
| parser.add_argument( | ||
| '--timeout', | ||
| type=int, | ||
| default=GLOBAL_TIMEOUT, | ||
| help=( | ||
| f'Maximum number of seconds to wait for each docker-compose process ' | ||
| f'(build, run, or field-compare) before killing it and marking the ' | ||
| f'test as failed. Defaults to {GLOBAL_TIMEOUT} seconds. ' | ||
| f'Increase this value for slow machines or large simulations; ' | ||
| f'decrease it to catch hanging tests faster.' | ||
| ) | ||
| ) |
There was a problem hiding this comment.
The new --timeout flag controls the docker-compose process timeout, but issue #402 requests overriding preCICE's in precice-config.xml for selected tests. If this PR intends to close #402, it likely needs additional YAML plumbing + XML modification (or the PR title/issue linkage should be updated to avoid implying #402 is fixed).
| partitioned_heat_conduction_test: | ||
| tutorials: | ||
| - path: partitioned-heat-conduction | ||
| case_combination: | ||
| - dirichlet-fenics | ||
| - neumann-fenics | ||
| reference_result: ./partitioned-heat-conduction/reference-results/dirichlet-fenics_neumann-fenics.tar.gz | ||
| - path: partitioned-heat-conduction | ||
| case_combination: | ||
| - dirichlet-nutils | ||
| - neumann-nutils | ||
| reference_result: ./partitioned-heat-conduction/reference-results/dirichlet-nutils_neumann-nutils.tar.gz | ||
| - path: partitioned-heat-conduction | ||
| case_combination: | ||
| - dirichlet-openfoam | ||
| - neumann-openfoam | ||
| reference_result: ./partitioned-heat-conduction/reference-results/dirichlet-openfoam_neumann-openfoam.tar.gz |
There was a problem hiding this comment.
This new test suite references reference_result archives that are not present in the repository (partitioned-heat-conduction/reference-results currently only contains .gitkeep). As-is, running this suite will always fail until the corresponding .tar.gz files (likely Git LFS pointers) are added, or the suite is gated/removed until reference results land.
| partitioned_heat_conduction_test: | |
| tutorials: | |
| - path: partitioned-heat-conduction | |
| case_combination: | |
| - dirichlet-fenics | |
| - neumann-fenics | |
| reference_result: ./partitioned-heat-conduction/reference-results/dirichlet-fenics_neumann-fenics.tar.gz | |
| - path: partitioned-heat-conduction | |
| case_combination: | |
| - dirichlet-nutils | |
| - neumann-nutils | |
| reference_result: ./partitioned-heat-conduction/reference-results/dirichlet-nutils_neumann-nutils.tar.gz | |
| - path: partitioned-heat-conduction | |
| case_combination: | |
| - dirichlet-openfoam | |
| - neumann-openfoam | |
| reference_result: ./partitioned-heat-conduction/reference-results/dirichlet-openfoam_neumann-openfoam.tar.gz |
| @@ -0,0 +1,11 @@ | |||
| # Reference results for partitioned-heat-conduction are stored as Git LFS archives. | |||
| # | |||
| # To generate them locally, run from tutorials/tools/tests: | |||
There was a problem hiding this comment.
The instructions say to run from "tutorials/tools/tests", but in this repository the path is just "tools/tests" from the repo root. Please update this path to avoid confusing users.
| # To generate them locally, run from tutorials/tools/tests: | |
| # To generate them locally, run from tools/tests: |
Summary
This PR contains contributions developed as part of the GSoC 2026 entry test
for the "System Tests Improvements" project.
1. Add
partitioned-heat-conductionto system tests (entry test)partitioned-heat-conduction/metadata.yamldefining 2 participants(Dirichlet, Neumann) and 6 cases across fenics-adapter, nutils-adapter,
and openfoam-adapter.
partitioned_heat_conduction_testsuite intests.yamlcoveringthe three homogeneous solver combinations.
python print_case_combinations.py(output below).print_case_combinations.py output for partitioned-heat-conduction
partitioned-heat-conduction:(dirichlet-fenics, neumann-fenics)
(dirichlet-fenics, neumann-nutils)
(dirichlet-fenics, neumann-openfoam)
(dirichlet-nutils, neumann-fenics)
(dirichlet-nutils, neumann-nutils)
(dirichlet-nutils, neumann-openfoam)
(dirichlet-openfoam, neumann-fenics)
(dirichlet-openfoam, neumann-nutils)
(dirichlet-openfoam, neumann-openfoam)
2. Fix #404 — user-friendly error when reference results are missing
__unpack_reference_results()now checks for the archive before opening itand raises a
FileNotFoundErrorwith a clear message pointing togenerate_reference_results.py._run_field_compare()catches it andreturns a clean
FieldCompareResult(exit_code=1)instead of a raw exception.3. Fix #674 — properly escape GitHub step summary markdown
Separates the Markdown summary output from the fixed-width plain-text terminal
output. Adds
_escape_markdown_cell()that escapes|,`,*,_,~to prevent broken table cells. Success/failure now renders as:white_check_mark:/:x:icons.4. Fix #402 — configurable
--timeoutper runAdds
timeout: int = GLOBAL_TIMEOUTfield toSystemtestdataclass and a--timeoutCLI argument tosystemtests.py. All threeprocess.communicate()calls (build, run, field-compare) useself.timeout.Testing
python -c "import ast; ast.parse(open('tools/tests/systemtests/Systemtest.py').read())"— OKpython -c "import ast; ast.parse(open('tools/tests/systemtests.py').read())"— OKpython tools/tests/print_case_combinations.py— partitioned-heat-conduction parsed correctly