Add run-before / run-after hooks for system tests#849
Conversation
Fixes precice#827. Per-test shell commands run in the copied tutorial directory before Docker build (run-before) or after simulation (run-after). Wire water-hammer and partitioned-pipe-multiscale with ./set-case.sh 1d3d.
MakisH
left a comment
MakisH
left a comment
There was a problem hiding this comment.
Thanks for implementing! Here are some comments before testing on GHA.
| @@ -0,0 +1 @@ | |||
| - Add optional `run-before` and `run-after` hooks in `tests.yaml` for system test setup ([#827](https://github.com/precice/tutorials/pull/849)). | |||
There was a problem hiding this comment.
| - Add optional `run-before` and `run-after` hooks in `tests.yaml` for system test setup ([#827](https://github.com/precice/tutorials/pull/849)). | |
| - Added optional `run-before` and `run-after` hooks in `tests.yaml` for system test setup ([#849](https://github.com/precice/tutorials/pull/849)). |
| run_before: Optional[str] = None | ||
| run_after: Optional[str] = None |
There was a problem hiding this comment.
Instead of the Optional, you need the | syntax. Also in the _run_hook.
| """ | ||
| Run a shell command in the copied tutorial directory (e.g. run-before / run-after). | ||
| """ |
There was a problem hiding this comment.
The overall concept needs to be explained in the PR description.
| if result.stdout: | ||
| logging.debug(f"{stage} stdout for {self}:\n{result.stdout.rstrip()}") | ||
| if result.stderr: | ||
| logging.debug(f"{stage} stderr for {self}:\n{result.stderr.rstrip()}") |
There was a problem hiding this comment.
Why distinguishing between stdout and stderr here? What is the motivation behind this?
| def _validate_hook_command(field_name: str, value, tutorial: Tutorial) -> Optional[str]: | ||
| if value is None: | ||
| return None | ||
| if not isinstance(value, str) or not value.strip(): | ||
| raise ValueError( | ||
| f"{field_name} must be a non-empty string for tutorial '{tutorial}', got {value!r}") | ||
| return value |
There was a problem hiding this comment.
Why is a dedicated function needed to check if field_name is non-empty?
Why is it called field_name?
Also, if it is empty, it should just do nothing, right? That would make the code simpler.
|
|
||
| Use the `max_time` or `max_time_windows` parameters to restrict the runtime of the test to the first few coupling time windows, to save time. Aim for a runtime of less than a minute (assuming cached components), if possible. | ||
|
|
||
| Some tutorials require setup before the simulation (e.g. switching configuration files). Use optional `run-before` and `run-after` fields in `tests.yaml` to run shell commands in the copied tutorial directory after copying and before Docker build (`run-before`), or after the simulation and before field comparison (`run-after`). Example: |
There was a problem hiding this comment.
Why before the Docker build? I would expect that to be after building and before running. Is there a particular use case or complication I overlook?
For the set-case, it should make no difference.
2 participants
Summary
run-before/run-afterintests.yamlrun-before, beforemax_time), after simulation (run-after, before fieldcompare)water-hammerandpartitioned-pipe-multiscalewith./set-case.sh 1d3dfixes Run a command before starting a test #827
Test plan