Update URM Test Runner - #568
Conversation
Srikanth Muppandam (smuppand)
left a comment
Srikanth Muppandam (smuppand)
left a comment
There was a problem hiding this comment.
Kartik Nema (@kartnema) Please update the commit message to reflect what changes are being introduced as well.
| COMMON_CONFIGS_DIR="$URM_CONFIG_DIR/common" | ||
| TEST_CONFIGS_DIR="$URM_CONFIG_DIR/tests/configs" | ||
| TEST_NODES_DIR="$URM_CONFIG_DIR/tests/nodes" | ||
| URM_COMMON_CONFIG_DIR="${URM_COMMON_CONFIG_DIR:-/etc/urm}" |
There was a problem hiding this comment.
URM_CONFIG_DIR is a documented public override, but this replaces it with URM_COMMON_CONFIG_DIR and silently ignores existing callers.
A CI job using URM_CONFIG_DIR=/opt/urm will now inspect /etc/urm and /usr/share/urm instead, producing an incorrect SKIP.
Preserve URM_CONFIG_DIR as the compatibility fallback for all three roots, while allowing the new per-root overrides to take precedence.
There was a problem hiding this comment.
I am not sure I understood it totally.
Could you please explain this part:
"Preserve URM_CONFIG_DIR as the compatibility fallback for all three roots, while allowing the new per-root overrides to take precedence."
There was a problem hiding this comment.
I am not sure I understood it totally. Could you please explain this part: "Preserve URM_CONFIG_DIR as the compatibility fallback for all three roots, while allowing the new per-root overrides to take precedence."
It means keep old users working while adding finer control. Before this PR, one variable controlled every URM asset path:
URM_CONFIG_DIR=/opt/urm
The runner then looked for:
/opt/urm/common
/opt/urm/tests/configs
/opt/urm/tests/nodes
This PR removes that variable and instead uses three new variables:
URM_COMMON_CONFIG_DIR
URM_TESTS_CONFIG_DIR
URM_TEST_NODES_DIR
So an existing CI job that sets URM_CONFIG_DIR=/opt/urm is silently ignored. That is the compatibility break.
For example:
URM_COMMON_CONFIG_DIR="${URM_COMMON_CONFIG_DIR:-${URM_CONFIG_DIR:-/etc/urm}}"
URM_TESTS_CONFIG_DIR="${URM_TESTS_CONFIG_DIR:-${URM_CONFIG_DIR:-/usr/share/urm}}"
URM_TEST_NODES_DIR="${URM_TEST_NODES_DIR:-${URM_CONFIG_DIR:-/usr/share/urm}}"
Result:
- Existing URM_CONFIG_DIR=/opt/urm users continue to work.
- A user can override only one location when packages split assets:
URM_TEST_NODES_DIR=/var/lib/urm ./run.sh
- When neither is set, the runner uses the package defaults.
There was a problem hiding this comment.
Got it, I have made some changes to fix this. please check
|
|
||
| # nodes will be available either in /var/lib or /usr/share | ||
| URM_TEST_NODES_DIR="${URM_TEST_NODES_DIR:-/usr/share/urm}" | ||
| if [ -d "/var/lib/urm/tests" ]; then |
There was a problem hiding this comment.
This unconditionally overrides an explicitly supplied URM_TEST_NODES_DIR whenever /var/lib/urm/tests exists. It also selects /var/lib solely because the directory exists, even if it is empty while the package-provided /usr/share/urm/tests/nodes is valid.
Resolve candidates in priority order: explicit override first, then valid populated runtime location, then valid package location. Do not replace an operator’s explicit selection.
| # Setup: Move all nodes to /var/lib/urm/tests to have a consistent start point. | ||
| TEST_NODES_DEST_DIR="/run/urm/tests/" | ||
| mkdir -p "$TEST_NODES_DEST_DIR" | ||
| cp -r "$TEST_NODES_DIR" "$TEST_NODES_DEST_DIR" |
There was a problem hiding this comment.
The new copy is not consumed by either run_one or the test binaries: the runner validates TEST_NODES_DIR, copies it to /run/urm/tests/nodes, then invokes the binaries without an argument, environment variable, or working-directory change that points to /run.
The comment says /var/lib/urm/tests, which is a third, different path. This adds state mutation without fixing non-standard discovery. Remove the copy unless the binary has a documented runtime-node override; if it does, pass that exact destination explicitly and verify the copy before starting tests.
| done | ||
|
|
||
| # Cleanup, delete the copied nodes | ||
| rm -rf "$TEST_NODES_DEST_DIR" |
There was a problem hiding this comment.
rm -rf /run/urm/tests/ deletes a fixed shared directory rather than test-owned state, and it is not protected by the existing exit trap. A failure, interrupt, or pre-existing daemon-owned content can leave stale data or delete unrelated runtime nodes.
Do not recursively remove this shared path. If a temporary copy is genuinely required, create a test-owned directory with mktemp -d, register cleanup immediately with trap, and remove only that owned directory. If the binary requires a fixed path, snapshot and restore only the state created by this test.
|
|
||
| ### 3) Test test nodes | ||
| `/etc/urm/tests/nodes` must exist and be non‑empty for **`/usr/bin/UrmIntegrationTests`** and **`/usr/bin/UrmComponentTests`**. If missing/empty → **SKIP only that suite**. | ||
| `/usr/share/tests/nodes` must exist and be non‑empty for **`/usr/bin/UrmIntegrationTests`** and **`/usr/bin/UrmComponentTests`**. If missing/empty → **SKIP only that suite**. |
There was a problem hiding this comment.
The documentation says /usr/share/tests/nodes, while the current code checks /usr/share/urm/tests/nodes; it also still documents the removed URM_CONFIG_DIR interface below. This directs users to the wrong path and makes overrides appear supported when they are ignored. Document the actual candidate order and the retained compatibility/new override variables.
- Update test configs and test nodes selection order to reflect the movement of these files to /usr/share/urm/tests in latest URM release - Stage the test nodes in /run/urm/tests/ temporarily during the course of the test execution and clean them up once done. Signed-off-by: Kartik Nema <kartnema@qti.qualcomm.com>
98fe447 to
f7c7f93
Compare
|
Please address the comments where you’ve already provided fixes so they’re easier to review. |
| # immediately so it runs on exit, interrupt, or termination. | ||
| RUNTIME_NODES_DIR="/run/urm/tests/nodes" | ||
| if [ "$TEST_NODES_OK" -eq 1 ]; then | ||
| mkdir -p "$RUNTIME_NODES_DIR" |
There was a problem hiding this comment.
mkdir -p accepts an existing /run/urm/tests/nodes, but this test does not establish that it created or exclusively owns that directory. A previous interrupted run, another service, or a manual diagnostic can leave nodes there; the subsequent copy merges its files with stale content and the exit trap later deletes the entire directory.
That can produce invalid test inputs or remove state outside this run. Require an absent/test-owned destination before staging, or snapshot and restore pre-existing state. The cleanup must remove only assets this invocation created.
| RUNTIME_NODES_DIR="/run/urm/tests/nodes" | ||
| if [ "$TEST_NODES_OK" -eq 1 ]; then | ||
| mkdir -p "$RUNTIME_NODES_DIR" | ||
| if cp -r "$TEST_NODES_DIR/"* "$RUNTIME_NODES_DIR/"; then |
There was a problem hiding this comment.
The copy occurs before the cleanup trap is installed and its destination is not cleared or made atomic. If cp partially copies and then fails, TEST_NODES_OK=0 skips the suites but leaves a partial /run/urm/tests/nodes; a later run can merge with and execute against that stale partial set. Register a cleanup handler immediately after establishing owned staging state, and stage atomically, for example, copy to a private temporary directory, validate it, then publish it only when the fixed runtime path is safely owned.
| log_info "[NODES] Staged test nodes from $TEST_NODES_DIR to $RUNTIME_NODES_DIR" | ||
| # Extend the existing trap to also clean up the staged nodes. | ||
| if [ "$lock_flock" -eq 1 ]; then | ||
| trap 'rm -rf "$RUNTIME_NODES_DIR"; exec 9>&-' EXIT INT TERM |
There was a problem hiding this comment.
The trap uses rm -rf "$RUNTIME_NODES_DIR" for every successful staging run, including when mkdir -p reused a pre-existing directory. This contradicts the comment that cleanup “removes only the directory we created.” Track ownership explicitly and fail/skip rather than deleting a path that was already present; otherwise an interrupted or concurrent system component can lose its runtime nodes.
2 participants
movement of these files to /usr/share/urm/tests in latest URM
release
course of the test execution and clean them up once done.