test: use a unique temp dir in test_variable_in_filter#2368
Open
eran132 wants to merge 2 commits into
Open
Conversation
The test wrote its fixture to the fixed path /tmp/key_file and the OVAL definition hardcoded /tmp as the file_object/file_state path. A predictable, world-accessible path in a shared directory races with other users or parallel test runs and lets an unrelated /tmp/key_file influence the result. Follow the pattern already used by test_pcre_nonutf_characters: put a TEMP_DIR_PLACEHOLDER in the OVAL definition, copy it to a temporary file, sed-substitute a per-run `mktemp -d` directory, and create the key_file fixture there. The filename pattern and evaluation logic are unchanged. Clean up the temporary directory at the end. Fixes OpenSCAP#1924 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR makes the test_variable_in_filter unit test more hermetic by avoiding hard-coded /tmp paths and instead using a dynamically created temporary directory.
Changes:
- Replaced
/tmpin the OVAL XML with a placeholder path. - Updated the shell test to create an isolated temp directory, patch the XML, and run the evaluation against the patched XML.
- Adjusted cleanup to remove generated temp artifacts.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/API/OVAL/unittests/test_variable_in_filter.xml | Replaces hard-coded /tmp with TEMP_DIR_PLACEHOLDER for runtime substitution. |
| tests/API/OVAL/unittests/test_variable_in_filter.sh | Creates temp dir/file, substitutes placeholder in XML, runs eval against temp XML, and cleans up. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+4
to
+11
| set -e | ||
| set -o pipefail | ||
|
|
||
| oval_def=`mktemp` | ||
| result=`mktemp` | ||
| stdout=`mktemp` | ||
| stderr=`mktemp` | ||
| echo "secret_key" > /tmp/key_file | ||
| temp_dir=`mktemp -d` |
Comment on lines
+21
to
+22
| rm -f "$oval_def" "$result" "$stdout" "$stderr" | ||
| rm -rf "$temp_dir" |
| echo "secret_key" > /tmp/key_file | ||
| temp_dir=`mktemp -d` | ||
| cp "$srcdir/test_variable_in_filter.xml" "$oval_def" | ||
| sed -i "s;TEMP_DIR_PLACEHOLDER;$temp_dir;" "$oval_def" |
Comment on lines
+7
to
+11
| oval_def=`mktemp` | ||
| result=`mktemp` | ||
| stdout=`mktemp` | ||
| stderr=`mktemp` | ||
| echo "secret_key" > /tmp/key_file | ||
| temp_dir=`mktemp -d` |
Address review feedback: - Register a cleanup() handler with `trap ... EXIT` so the temporary files and directory are removed even when an assertion fails under `set -e`, instead of relying on a final rm that is skipped on failure. - Switch the backtick command substitutions to $(...) for readability, matching the sibling test_pcre_nonutf_characters test.
|
Author
|
Thanks for the review. Addressed in 47338aa:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Problem
tests/API/OVAL/unittests/test_variable_in_filter.shwrote its fixture to afixed path,
/tmp/key_file, and the OVAL definition hardcoded/tmpas thefile_object/file_statepath (#1924). A predictable, world-writable pathin a shared directory:
/tmp/key_fileleft by someone else affect theresult.
Fix
Adopt the isolation pattern already used by
test_pcre_nonutf_characters.sh:TEMP_DIR_PLACEHOLDERtoken instead of/tmp.sed-substitutes a per-runmktemp -ddirectory, and creates thekey_filefixture inside it.The filename pattern (
^key_file$) and the evaluation logic are unchanged, sothe test asserts exactly the same OVAL behavior — just in an isolated location.
Verification
This is a test-only change. I verified mechanically (the full suite runs in CI):
bash -npasses on the script.sedsubstitution leaves zeroTEMP_DIR_PLACEHOLDERoccurrences andrewrites both
<ns3:path>elements to the unique temp dir.key_filefixture is created in the temp dir as expected.Fixes #1924