Add foremanctl restore test cases in robottelo - #22300
Conversation
Reviewer's GuideExtends foremanctl backup test module to cover the new restore command with a shared backup helper, one end-to-end positive backup/restore test, and three focused restore validation/negative tests. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/foreman/foremanctl/test_foremanctl_backup.py" line_range="127-129" />
<code_context>
+ f'Some required backup files are missing. Expected: {expected_files}, Found: {files}'
+ )
+
+ module_target_sat.execute('rm -rf /var/lib/pulp/media/artifact')
+
+ result = module_target_sat.execute(
</code_context>
<issue_to_address>
**suggestion (testing):** Assert that pulp artifacts are actually removed before restore to prove restore repopulates them
After the `rm -rf`, add a check that the artifact directory is actually empty (for example, by asserting `wc -l` is 0). This ensures the later `> 0` assertion clearly demonstrates that restore repopulates the artifacts, and avoids false positives if the delete fails or the path changes.
```suggestion
result = module_target_sat.execute('rm -rf /var/lib/pulp/media/artifact')
assert result.status == 0, (
f'Failed to remove pulp artifacts before restore:\n{result.stdout}\n{result.stderr}'
)
result = module_target_sat.execute(
'test -d /var/lib/pulp/media/artifact && '
'find /var/lib/pulp/media/artifact -type f | wc -l || echo 0'
)
artifact_count = int(result.stdout.strip() or 0)
assert artifact_count == 0, (
f'Expected no pulp artifacts before restore, but found {artifact_count}'
)
result = module_target_sat.execute(
```
</issue_to_address>
### Comment 2
<location path="tests/foreman/foremanctl/test_foremanctl_backup.py" line_range="154" />
<code_context>
+ )
+
+
+def test_positive_restore_validate(module_target_sat, setup_backup_tests):
+ """Verify foremanctl restore --validate checks backup integrity without making changes
+
</code_context>
<issue_to_address>
**issue (testing):** Test docstring claims no changes on `--validate`, but the test does not verify system state
The test currently only checks `status == 0`, so it doesn’t verify the “no changes” guarantee described in the docstring. Please either assert that a representative piece of system state (e.g., config checksum, DB value, or `foremanctl health` output) is unchanged before/after `--validate`, or soften the docstring so it doesn’t claim behavior the test doesn’t cover.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| module_target_sat.execute('rm -rf /var/lib/pulp/media/artifact') | ||
|
|
||
| result = module_target_sat.execute( |
There was a problem hiding this comment.
suggestion (testing): Assert that pulp artifacts are actually removed before restore to prove restore repopulates them
After the rm -rf, add a check that the artifact directory is actually empty (for example, by asserting wc -l is 0). This ensures the later > 0 assertion clearly demonstrates that restore repopulates the artifacts, and avoids false positives if the delete fails or the path changes.
| module_target_sat.execute('rm -rf /var/lib/pulp/media/artifact') | |
| result = module_target_sat.execute( | |
| result = module_target_sat.execute('rm -rf /var/lib/pulp/media/artifact') | |
| assert result.status == 0, ( | |
| f'Failed to remove pulp artifacts before restore:\n{result.stdout}\n{result.stderr}' | |
| ) | |
| result = module_target_sat.execute( | |
| 'test -d /var/lib/pulp/media/artifact && ' | |
| 'find /var/lib/pulp/media/artifact -type f | wc -l || echo 0' | |
| ) | |
| artifact_count = int(result.stdout.strip() or 0) | |
| assert artifact_count == 0, ( | |
| f'Expected no pulp artifacts before restore, but found {artifact_count}' | |
| ) | |
| result = module_target_sat.execute( |
|
|
PRT Result |
|
Can we cover this flow: |
41c269c to
7afc61f
Compare
|
Can you explain what does mean by "Change content" here in the workflow, do you meant sync the additional repos? |
|
PRT Result |
|
PRT Result |
|
Automated PostgreSQL upgrades during restore processes cause timeouts by failing to wait for service dependencies. |
rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
7afc61f to
cd88a1c
Compare
@sjha4 as per the suggestion and offline discussion, I have updated |
rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
Problem Statement
The foremanctl restore command has been implemented upstream (theforeman/foremanctl#549) but has no test coverage in robottelo. The existing
test_foremanctl_backup.pyonly covers the backup functionality.https://redhat.atlassian.net/browse/SAT-44898
Solution
Added 4 new test cases and a shared helper to tests/foreman/foremanctl/test_foremanctl_backup.py
test_positive_backup_restore(E2E): Full backup → restore with --force → health check → content verification (custom repo, RH repo, pulp artifacts)test_positive_restore_validate: Validates --validate flag checks backup integrity without modifying the systemtest_negative_restore_baddir: Verifies restore fails with a non-existing backup directorytest_negative_restore_no_force: Verifies restore fails without --force on an existing deployment (safety check)_create_backup(): Shared helper to avoid duplicating backup logic across restore testsRelated Issues
PRT test Cases example
Summary by Sourcery
Add test coverage for foremanctl restore alongside existing backup tests.
Tests:
foremanctl restore --validatechecks backup integrity without modifying the system.