Add test for foremanctl --pulp-log-level - #22316
Open
Alleny244 wants to merge 1 commit into
Open
Conversation
Contributor
Reviewer's GuideAdds a focused integration test to validate that the new foremanctl --pulp-log-level option is persisted to parameters.yaml and correctly applied to the pulp-api container and its logging behavior. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Alleny244
added
Stream
Contributor
Author
|
trigger: test-robottelo |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The test description mentions verifying
foremanctl deploy --helplists all supported Pulp log levels, but the implementation only exercises--pulp-log-level=error; consider adding an explicit assertion on the help output to cover that behavior. - The
journalctl --since "-2 min" | grep -i ":DEBUG:"check combined withhammer pingmay be timing-sensitive and brittle; you might want to widen the time window or make the DEBUG check more robust to avoid flakes. - In the parameters check you mix attribute and dict-style access (
params.pulp_log_levelvsparams.get("pulp_log_level")); standardizing on one access pattern would make the test clearer and less error-prone.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The test description mentions verifying `foremanctl deploy --help` lists all supported Pulp log levels, but the implementation only exercises `--pulp-log-level=error`; consider adding an explicit assertion on the help output to cover that behavior.
- The `journalctl --since "-2 min" | grep -i ":DEBUG:"` check combined with `hammer ping` may be timing-sensitive and brittle; you might want to widen the time window or make the DEBUG check more robust to avoid flakes.
- In the parameters check you mix attribute and dict-style access (`params.pulp_log_level` vs `params.get("pulp_log_level")`); standardizing on one access pattern would make the test clearer and less error-prone.
## Individual Comments
### Comment 1
<location path="tests/foreman/cli/test_logging.py" line_range="465-474" />
<code_context>
assert 'log_level' not in params, 'log_level still present in parameters after reset'
+
+
+@pytest.mark.foremanctl
+def test_positive_foremanctl_pulp_log_level(module_target_sat):
+ """Verify foremanctl deploy --pulp-log-level persists and applies to Pulp services.
+
+ :id: de48bbb5-7a0f-48fd-a984-98b26dad45e1
+
+ :steps:
+ 1. Deploy with --pulp-log-level=error
+ 2. Verify pulp_log_level is persisted in parameters file
+ 3. Verify PULP_LOG_LEVEL=error is applied on pulp-api
+ 4. Trigger Pulp activity and verify DEBUG is absent from pulp-api journal
+
+ :expectedresults:
+ 1. pulp_log_level=error is persisted correctly
+ 2. pulp-api environment has PULP_LOG_LEVEL=error
</code_context>
<issue_to_address>
**suggestion (testing):** Test does not validate `foremanctl deploy --help` output for `--pulp-log-level` options as described in the PR
The test currently covers deploy behavior and configuration only. Please add assertions that `foremanctl deploy --help` exits successfully and that its help text includes the `--pulp-log-level` option and all expected log level choices (`debug`, `info`, `warning`, `error`, `critical`), so the CLI interface is also validated as described in the PR.
</issue_to_address>
### Comment 2
<location path="tests/foreman/cli/test_logging.py" line_range="505-509" />
<code_context>
+ )
+
+ # With level=error, DEBUG must not appear (ERROR lines are not guaranteed on happy path)
+ sat.execute('hammer ping')
+ result = sat.execute(
+ r'journalctl --no-pager -u pulp-api.service --since "-2 min" | grep -i ":DEBUG:"'
+ )
+ assert result.status != 0, (
+ 'DEBUG messages found in pulp-api.service journal when pulp_log_level=error'
+ )
</code_context>
<issue_to_address>
**suggestion (testing):** Reliance on `grep -i ":DEBUG:"` exit code may be brittle; consider a more explicit assertion on journal output
This check depends on `grep -i ":DEBUG:"` returning non-zero, which tightly couples the test to the current log format and grep’s exit behavior. Instead, capture the `journalctl` output in Python and assert there are no matching DEBUG entries, so you can clearly distinguish a `journalctl` failure from the absence of DEBUG logs and avoid format-dependent behavior. You might also widen the `--since` range slightly to avoid flakiness if `hammer ping` is slow.
Suggested implementation:
```python
# With level=error, DEBUG must not appear (ERROR lines are not guaranteed on happy path)
sat.execute('hammer ping')
journal = sat.execute(
'journalctl --no-pager -u pulp-api.service --since "-5 min"'
)
assert journal.status == 0, (
f'Failed to read pulp-api.service journal when pulp_log_level=error:\n{journal.stderr}'
)
assert 'debug' not in journal.stdout.lower(), (
'DEBUG messages found in pulp-api.service journal when pulp_log_level=error'
)
assert 'log_level' not in params, 'log_level still present in parameters after reset'
```
1. If other parts of the test file assume the old `grep`-based behavior or reuse similar patterns, consider updating them to follow this direct-output assertion style for consistency.
2. If `sat.execute` raises on non-zero exit codes instead of just returning `status`, you may need to adjust the `assert journal.status == 0` line to match the helper’s actual behavior (e.g., catching exceptions or using a different attribute).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+465
to
+474
| @pytest.mark.foremanctl | ||
| def test_positive_foremanctl_pulp_log_level(module_target_sat): | ||
| """Verify foremanctl deploy --pulp-log-level persists and applies to Pulp services. | ||
|
|
||
| :id: de48bbb5-7a0f-48fd-a984-98b26dad45e1 | ||
|
|
||
| :steps: | ||
| 1. Deploy with --pulp-log-level=error | ||
| 2. Verify pulp_log_level is persisted in parameters file | ||
| 3. Verify PULP_LOG_LEVEL=error is applied on pulp-api |
Contributor
There was a problem hiding this comment.
suggestion (testing): Test does not validate foremanctl deploy --help output for --pulp-log-level options as described in the PR
The test currently covers deploy behavior and configuration only. Please add assertions that foremanctl deploy --help exits successfully and that its help text includes the --pulp-log-level option and all expected log level choices (debug, info, warning, error, critical), so the CLI interface is also validated as described in the PR.
Comment on lines
+505
to
+509
| sat.execute('hammer ping') | ||
| result = sat.execute( | ||
| r'journalctl --no-pager -u pulp-api.service --since "-2 min" | grep -i ":DEBUG:"' | ||
| ) | ||
| assert result.status != 0, ( |
Contributor
There was a problem hiding this comment.
suggestion (testing): Reliance on grep -i ":DEBUG:" exit code may be brittle; consider a more explicit assertion on journal output
This check depends on grep -i ":DEBUG:" returning non-zero, which tightly couples the test to the current log format and grep’s exit behavior. Instead, capture the journalctl output in Python and assert there are no matching DEBUG entries, so you can clearly distinguish a journalctl failure from the absence of DEBUG logs and avoid format-dependent behavior. You might also widen the --since range slightly to avoid flakiness if hammer ping is slow.
Suggested implementation:
# With level=error, DEBUG must not appear (ERROR lines are not guaranteed on happy path)
sat.execute('hammer ping')
journal = sat.execute(
'journalctl --no-pager -u pulp-api.service --since "-5 min"'
)
assert journal.status == 0, (
f'Failed to read pulp-api.service journal when pulp_log_level=error:\n{journal.stderr}'
)
assert 'debug' not in journal.stdout.lower(), (
'DEBUG messages found in pulp-api.service journal when pulp_log_level=error'
)
assert 'log_level' not in params, 'log_level still present in parameters after reset'- If other parts of the test file assume the old
grep-based behavior or reuse similar patterns, consider updating them to follow this direct-output assertion style for consistency. - If
sat.executeraises on non-zero exit codes instead of just returningstatus, you may need to adjust theassert journal.status == 0line to match the helper’s actual behavior (e.g., catching exceptions or using a different attribute).
Collaborator
|
PRT Result |
Satellite-QE
added
the
PRT-Failed
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
2 participants
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 Statement
foremanctladded the--pulp-log-leveloption in foremanctl#697 to configure Pulp logging (debug,info,warning,error,critical).Solution
Add
test_positive_foremanctl_pulp_log_levelto verify that:foremanctl deploy --helplists all supported Pulp log levels.foremanctl deploy --pulp-log-level=errorpersistspulp_log_levelinparameters.yaml.PULP_LOG_LEVEL=erroris applied to thepulp-apicontainer.pulp-apijournal contains noDEBUGmessages after generating activity.Related Issues
PRT Example
Summary by Sourcery
Tests: