Skip to content

test: use a unique temp dir in test_variable_in_filter#2368

Open
eran132 wants to merge 2 commits into
OpenSCAP:mainfrom
eran132:fix/test-variable-in-filter-tmpdir
Open

test: use a unique temp dir in test_variable_in_filter#2368
eran132 wants to merge 2 commits into
OpenSCAP:mainfrom
eran132:fix/test-variable-in-filter-tmpdir

Conversation

@eran132
Copy link
Copy Markdown

@eran132 eran132 commented Jun 6, 2026

Problem

tests/API/OVAL/unittests/test_variable_in_filter.sh wrote its fixture to a
fixed path, /tmp/key_file, and the OVAL definition hardcoded /tmp as the
file_object / file_state path (#1924). A predictable, world-writable path
in a shared directory:

  • races with parallel test runs (and with other users) on the same machine, and
  • lets a pre-existing, unrelated /tmp/key_file left by someone else affect the
    result.

Fix

Adopt the isolation pattern already used by test_pcre_nonutf_characters.sh:

  • The OVAL definition now uses a TEMP_DIR_PLACEHOLDER token instead of /tmp.
  • The test copies the definition to a temp file, sed-substitutes a per-run
    mktemp -d directory, and creates the key_file fixture inside it.
  • The temporary directory is removed on completion.

The filename pattern (^key_file$) and the evaluation logic are unchanged, so
the 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 -n passes on the script.
  • The sed substitution leaves zero TEMP_DIR_PLACEHOLDER occurrences and
    rewrites both <ns3:path> elements to the unique temp dir.
  • The resulting OVAL definition is still well-formed XML.
  • The key_file fixture is created in the temp dir as expected.

Fixes #1924

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>
Copilot AI review requested due to automatic review settings June 6, 2026 11:50
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 /tmp in 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.
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Jun 6, 2026

@eran132
Copy link
Copy Markdown
Author

eran132 commented Jun 6, 2026

Thanks for the review. Addressed in 47338aa:

  • Cleanup on failure: added a cleanup() handler registered with trap ... EXIT, so the temp files and mktemp -d directory are removed even if an assert_exists fails under set -e.
  • Backticks → $(...): switched, matching the sibling test_pcre_nonutf_characters.sh.
  • sed -i portability: kept as-is intentionally — the sibling test_pcre_nonutf_characters.sh uses the identical sed -i "s;...;...;" form and these OVAL unit tests run on GNU/Linux CI, so this stays consistent with the existing convention rather than introducing a divergent in-place-edit approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

API/OVAL/unittests/test_variable_in_filter.sh uses /tmp/key_file

2 participants