Refactor tests (suggestion) #3228
Open
Conversation
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
Contributor
Reviewer's GuideRefactors host app MQ service tests to use a single parametrized test suite that covers all application types (advisor, vulnerability, patch, remediations, compliance, malware, image_builder) for upsert, update, and batch-processing behavior, using shared test data configuration and generic assertions per model. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
test_batch_processing, relying onfirst_field = list(fields_to_verify.keys())[0]makes the assertion a bit fragile and only checks one field; consider iterating over allfields_to_verifyto validate both hosts’ persisted data more thoroughly and avoid depending on dict key order. - The comment
# Create data for second host (slightly different values)intest_batch_processingdoesn’t always hold becausemax(1, value - 2)can leave the value unchanged (e.g. whenvalue == 1); either adjust the transformation to guarantee a different value or update the comment to match the behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `test_batch_processing`, relying on `first_field = list(fields_to_verify.keys())[0]` makes the assertion a bit fragile and only checks one field; consider iterating over all `fields_to_verify` to validate both hosts’ persisted data more thoroughly and avoid depending on dict key order.
- The comment `# Create data for second host (slightly different values)` in `test_batch_processing` doesn’t always hold because `max(1, value - 2)` can leave the value unchanged (e.g. when `value == 1`); either adjust the transformation to guarantee a different value or update the comment to match the behavior.
## Individual Comments
### Comment 1
<location> `tests/test_host_app_mq_service.py:26-35` </location>
<code_context>
+ },
+ {"total_cves": 50, "critical_cves": 5, "high_severity_cves": 10},
+ ),
+ (
+ "patch",
+ HostAppDataPatch,
+ {"installable_advisories": 15, "template": "baseline-template", "rhsm_locked_version": "9.4"},
+ {"installable_advisories": 15, "template": "baseline-template", "rhsm_locked_version": "9.4"},
+ ),
+ (
+ "remediations",
+ HostAppDataRemediations,
+ {"remediations_plans": 7},
+ {"remediations_plans": 7},
+ ),
+ (
+ "compliance",
+ HostAppDataCompliance,
+ {"policies": 3, "last_scan": datetime.now(UTC).isoformat()},
</code_context>
<issue_to_address>
**issue (testing):** Compliance tests no longer assert that `last_scan` is persisted
Previously, `HostAppDataCompliance.last_scan` was asserted to be non-`None`, but in the refactored tests it’s no longer verified because `last_scan` is omitted from `fields_to_verify` for the `"compliance"` entry. To preserve coverage and catch regressions in timestamp handling, please either add `"last_scan"` to `fields_to_verify` (and assert equality) or add an explicit `assert app_data.last_scan is not None` in the compliance branch of `test_upsert_new_record` (analogous to the `last_updated` check for advisor).
</issue_to_address>
### Comment 2
<location> `tests/test_host_app_mq_service.py:249-253` </location>
<code_context>
- assert app_data is not None
- assert app_data.image_name == "rhel-9-base"
- assert app_data.image_status == "active"
+ # Verify first host has original data
+ first_field = list(fields_to_verify.keys())[0]
+ assert getattr(app_data1, first_field) == sample_data[first_field]
+ # Verify second host has different data (for numeric fields)
+ if isinstance(sample_data[first_field], int):
+ assert getattr(app_data2, first_field) == data2[first_field]
</code_context>
<issue_to_address>
**issue (testing):** Batch test does not actually validate second-host data for malware and image_builder
In `test_batch_processing`, only the first field in `fields_to_verify` is checked, and the second-host assertion only runs when that field is an `int`. For malware (`"last_status"`) and image_builder (`"image_name"`), the first field is a `str`, so we never validate host2’s values against `data2`, only that `app_data2` exists. To make these tests meaningful, iterate over all fields in `fields_to_verify` and, for numeric fields, assert that host2’s value equals `data2[field]` and differs from host1’s. That will properly verify per-host data isolation for all applications.
</issue_to_address>
### Comment 3
<location> `tests/test_host_app_mq_service.py:169-170` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 4
<location> `tests/test_host_app_mq_service.py:172-173` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid conditionals in tests. ([`no-conditionals-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-conditionals-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like conditionals, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 5
<location> `tests/test_host_app_mq_service.py:192-196` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 6
<location> `tests/test_host_app_mq_service.py:193-196` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid conditionals in tests. ([`no-conditionals-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-conditionals-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like conditionals, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 7
<location> `tests/test_host_app_mq_service.py:208-214` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 8
<location> `tests/test_host_app_mq_service.py:211-214` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid conditionals in tests. ([`no-conditionals-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-conditionals-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like conditionals, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 9
<location> `tests/test_host_app_mq_service.py:227-229` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 10
<location> `tests/test_host_app_mq_service.py:228-229` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid conditionals in tests. ([`no-conditionals-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-conditionals-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like conditionals, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 11
<location> `tests/test_host_app_mq_service.py:253-254` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid conditionals in tests. ([`no-conditionals-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-conditionals-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like conditionals, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</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
+249
to
+253
| # Verify first host has original data | ||
| first_field = list(fields_to_verify.keys())[0] | ||
| assert getattr(app_data1, first_field) == sample_data[first_field] | ||
| # Verify second host has different data (for numeric fields) | ||
| if isinstance(sample_data[first_field], int): |
Contributor
There was a problem hiding this comment.
issue (testing): Batch test does not actually validate second-host data for malware and image_builder
In test_batch_processing, only the first field in fields_to_verify is checked, and the second-host assertion only runs when that field is an int. For malware ("last_status") and image_builder ("image_name"), the first field is a str, so we never validate host2’s values against data2, only that app_data2 exists. To make these tests meaningful, iterate over all fields in fields_to_verify and, for numeric fields, assert that host2’s value equals data2[field] and differs from host1’s. That will properly verify per-host data isolation for all applications.
24 tasks
rodrigonull
force-pushed
the
RHINENG-22193
branch
2 times, most recently
from
54c3467 to
9603483
Compare
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.
This PR is being opened to suggest a refactor for
test_host_app_mq_service.pyfor PR #3204.Summary by Sourcery
Refactor host application message consumer tests to use a unified, parameterized suite covering all application types.
Tests: