cleanup test directories before running tests #826
Conversation
|
The PR title could be a bit more descriptive. Also, please squash the last two commits onto the first |
Edu92337
changed the title
Edu92337
force-pushed
the
functional-tests
branch
3 times, most recently
from
a8fe5ad to
f88c96e
Compare
|
|
||
| fi | ||
|
|
||
| # Clean existing data/logs directories before running the tests |
There was a problem hiding this comment.
Why remove this comment here?
Edu92337
force-pushed
the
functional-tests
branch
2 times, most recently
from
e8e8661 to
8345fa4
Compare
|
ACK 8345fa4 |
moisesPompilio
left a comment
moisesPompilio
left a comment
There was a problem hiding this comment.
Please squash the commits, because the comment adjustments in the file are not doc, they can be part of the fix.
Edu92337
force-pushed
the
functional-tests
branch
2 times, most recently
from
02392c9 to
217e47c
Compare
moisesPompilio
left a comment
moisesPompilio
left a comment
There was a problem hiding this comment.
You removed several comments, is there any reason?
|
|
||
| fi | ||
|
|
||
| # Clean existing data/logs directories before running the tests |
| # Detect if --preserve-data-dir is among args | ||
| # and forward args to uv |
There was a problem hiding this comment.
Why remove this comment here?
There was a problem hiding this comment.
I replaced the step-by-step inline comments with a general-purpose comment block at the top. But I can leave the comments in the code as well if you prefer.
There was a problem hiding this comment.
Here #826 (comment) I had asked for the removal of that comment that described the steps, saying it would be better to have a comment stating the file's intent, but I didn't ask for the removal of the comments on the code lines.
- Keep unconditional /data cleanup at start - Make /logs cleanup conditional on --preserve-data-dir flag
Edu92337
force-pushed
the
functional-tests
branch
from
217e47c to
42c63a9
Compare
|
ACK 42c63a9 |
|
ACK 42c63a9 |
4 participants
Related to #760 (mitigation)
Keep unconditional /data cleanup at start
Make /logs cleanup conditional on --preserve-data-dir flag
Prevents reusing old log state between test runs
Description and Notes
Currently, the cleanup of /logs directory happens at the END of test execution and only if tests pass. This causes:
Failed tests leave corrupted log state for next run
Tests may fail when run multiple times without manual cleanup
This PR provides a temporary mitigation by making /logs cleanup conditional on the --preserve-data-dir flag, while keeping /data unconditionally cleaned at start.
Note: This is a partial fix. Complete resolution will come in a follow-up PR after #742 is merged, where cleanup logic will be migrated to the Python runner for better control over test data management.
How to verify the changes you have done?
Test 1: Run tests multiple times
./tests/prepare.sh
./tests/run.sh
./tests/run.sh # Should work without manual cleanup
Test 2: Preserve logs for debugging
./tests/run.sh --preserve-data-dir
ls -la $FLORESTA_TEMP_DIR/logs # Logs preserved
Next Steps
After #742 is merged, I plan to submit a follow-up PR to:
Migrate cleanup logic to the Python test runner
Provide more granular control over test data management
Fully resolve #760
Contributor Checklist
I've followed the contribution guidelines
I've verified one of the following:
Ran just pcc (skipped - changes are bash-only, validated manually)
Ran just lint-features '-- -D warnings' && cargo test --release (not applicable)
Confirmed CI passed on my fork (will run after PR creation)
I've linked any related issue(s) in the sections above
Manual validation performed:
✅ Bash syntax check (bash -n)
✅ Logic tested with simulations
✅ --preserve-data-dir flag tested
✅ Consecutive test runs verified