-
Notifications
You must be signed in to change notification settings - Fork 115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add coverage for BZ1964539 #14519
Add coverage for BZ1964539 #14519
Conversation
trigger: test-robottelo |
trigger: test-robottelo |
PRT Result
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the test will cover what was in the BZ. My main concern is if this test is really necessary since we are only testing the one packages. I'd love to find a way to make this have a bigger impact with more than just one package. @lpramuk @jameerpathan111 @ehelms @evgeni @ekohl any thoughts around this?
I wonder what the best way of approaching this is. I'd imagine you can test this out without installing things which is a lot faster. How about this:
You may need to tune the package list and you can probably parametrize it, or even skip the Another approach would be to work with delivery to get this in a regular pipeline. They will investigate the failures of those and prevent it from even promoting in the first place. |
Why test using Also, Ewould has a point, it may be better to check for package presence somewhere else. My suggestion would be logs, the same way installer log is checked. Additionally, at the time when the logs are checked (whatever it is), you could maybe directly check for rpm presence. |
I would definitely like to make this bigger, that's why it is parametrized. Current solution uses the fixture sat_non_default_install which is common for more testcases in this module. So there's no significant additional time cost. As said in the BZ, the unwanted dependency does not usually come from the main package itself (satellite.*rpm), but from some other subcomponent/feature which is not installed by default. This is the reason why I chose the opposite approach, i.e. enable various non-default subcomponents/features, attempt to remove unwanted package(s) and check if it does not remove any of our packages. As a benefit we also test if the non-default subcomponents/features can be installed during the fixture setup. |
I tried to find a way to install packages in bulk, but
So I'd this is a way to install satellite and all optional plugins: We don't have an equivalent provides for Hammer or Smart Proxy plugins, so you're stuck with globbing the package name with Note that none of this needs actual installation of packages so you can really use a bare RHEL with the right repos set up. |
Currently |
I slightly updated test docstring to make it more clear that current solution tries to be as close as possible to customer experience. As reported in the BZ, the customer had an existing instance of Satellite and he could not remove potentially harmful package because it would also remove some satellite or foreman package(s). |
This PR covers the BZ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test fully covers the related BZ. Radek chose a different approach from what @ekohl suggested, but it still covers the product bug. The test will pass also in a situation when this dependency is not present in the system or if it got renamed. That is a downside of the current form of the test.
LGTM otherwise. I see no reason to block it at the moment.
(cherry picked from commit db5b3d5)
Add coverage for BZ1964539 (#14519) (cherry picked from commit db5b3d5) Co-authored-by: rmynar <[email protected]>
Adding a test case to cover BZ1964539
Satellite and any of it's dependencies should not require package nmap-ncat.
The test is parametrized, so it can be easily extended to test different packages.