Skip to content
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

Improved RpmPackage is_installed property logic. #759

Merged
merged 8 commits into from
May 26, 2024

Conversation

narmaku
Copy link
Contributor

@narmaku narmaku commented Apr 1, 2024

This new logic verifies if there is a "not installed" string in the stdout when "rpm -q %s" command returns 1.

As seen in bug #758, if rpm command failed because of other reasons (such as database corruption), the old implementation was returning True, making the end-user think that the package was installed, however it was never queried.

@narmaku
Copy link
Contributor Author

narmaku commented May 1, 2024

@CarstenGrohmann , @philpep since you are the most active collaborators/maintainers, would you mind checking this quick PR? Thanks!

@martinhoyer
Copy link
Contributor

lgtm, I'd just use f-strings, as testinfra requires Python>=3.9.
I'm also interested in learning about review process here, looks like there is quite a number of PRs without a response.

@CarstenGrohmann
Copy link
Contributor

LGTM, also. Please add a small test case.

testinfra/modules/package.py Outdated Show resolved Hide resolved
testinfra/modules/package.py Outdated Show resolved Hide resolved
@narmaku narmaku requested a review from philpep May 9, 2024 06:13
if result.succeeded:
return True

if "not installed" in result.stdout or not result.stdout.startswith(self.name):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can be pickier here and make the stdout match a regex, just to make sure there is actually a package with its version, etc. @philpep What do you think?

test/test_modules.py Outdated Show resolved Hide resolved
@CarstenGrohmann
Copy link
Contributor

CarstenGrohmann commented May 13, 2024

@narmaku: I suggest to simplify the test as a corrupt database is not necessary:

$ rpm -q not_installed_pkg
package not_installed_pkg is not installed
$ echo $?

result = self.run("rpm -q --quiet %s 2>&1", self.name)
if result.succeeded:
return True
elif result.failed and result.stdout == '':
Copy link
Contributor Author

@narmaku narmaku May 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: If return code is 1 but there is no output/error, it means the package is not installed
(for that we used --quiet option).

elif result.failed and result.stdout == '':
return False
else:
raise RuntimeError(f"Could not check if RPM package '{self.name}' is installed. {result.stdout}")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: If return code is 1 and there is stdout, that means there was an unexpected error, so we throw a RuntimeError here. (as we use --quiet there should not be any output when failing).

narmaku and others added 4 commits May 26, 2024 17:55
This new logic verifies if there is a "not installed" string in
the stdout when "rpm -q %s" command returns 1.

As seen in bug pytest-dev#758, if rpm command failed because of other reasons (such
as database corruption), the old implementation was returning True,
making the end-user think that the package was installed, however it was
never queried.
Also improved package check logic to be locales agnostic.
@philpep philpep force-pushed the fix_package_is_installed_prop branch from ff45060 to 94cb4d5 Compare May 26, 2024 15:55
Copy link
Contributor

@philpep philpep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I modified the test with a corrupted rpmdb and switched back to use run_test().

@philpep philpep merged commit 35d6dec into pytest-dev:main May 26, 2024
7 checks passed
@philpep
Copy link
Contributor

philpep commented May 26, 2024

Merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants