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.

Sign up for GitHub

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

Fix apt unit tests #6029

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Fix apt unit tests #6029

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a44d5ab
fix debian and Ubuntu version detection
dnegreira Sep 13, 2024
eff2add
Improve apt.test_provides unit test
dnegreira Sep 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions avocado/utils/distro.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,7 @@ class DebianProbe(Probe):

CHECK_FILE = "/etc/debian_version"
CHECK_FILE_DISTRO_NAME = "debian"
CHECK_VERSION_REGEX = re.compile(r"(\d+)\.(\d+)")
Copy link
Contributor

Choose a reason for hiding this comment

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

I've noticed that, for Debian stable, /etc/debian_version contains a numeric value, such as 12.7. On testing, because there is no numeric version number still determined, it contains trixie/sid.

I'm not sure it's a good idea to default to using code names instead of code names all over. It may be a requirement for testing, but, numeric has advantages such as being able to do comparisons. Or maybe I'm just not aware that Debian mostly disregards numeric versions.

My perception is that the best solution would be one where numeric values are given if available, and if not, code names as used as fallbacks.

Copy link
Author

@dnegreira dnegreira Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@clebergnu - you are correct here, we can indeed verify if we have /sid in /etc/debian_version or a float value to be able to do the comparison values here.

I need some guidance to achieve this since I don't want to break other things and have a predictable behaviour, or at least you know about it :-)

In your opinion, how do you see this being achieved so that we don't break other things in avocado? Would it be ok to just retrieve the string "sid" OR the full float (example in debian11 11.11 and debian12 12.7) by using an OR on the regex, which would mean we would either return a string or a float. I can verify that on the unit tests we retrieve either of them accordingly, but down the line I am not sure if you want to deal with it like that or other way.

An example of the regex I am thinking on having here

((.+)/(.+)|(\d+.\d+)) - would this approach be ok with you?

My concern is that we are either returning a string or a float here.

Copy link
Author

@dnegreira dnegreira Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

quick update that I have tried the above regexp I have proposed, and the unit tests below get in a stuck state, not able to further debug, not sure why, but seems that the solution of "ORing" in a regexp is not a good one.

unit-644-selftests/unit/utils/software_manager.py:Dpkg.test_is_valid: INTERRUPTED
unit-645-selftests/unit/utils/software_manager.py:Dpkg.test_is_not_valid: INTERRUPTED

CHECK_VERSION_REGEX = re.compile(r"(.+)/(.+)")


class UbuntuProbe(Probe):
Expand All @@ -399,7 +399,7 @@ class UbuntuProbe(Probe):
CHECK_FILE_CONTAINS = "ubuntu"
CHECK_FILE_DISTRO_NAME = "Ubuntu"
CHECK_VERSION_REGEX = re.compile(
r".*VERSION_ID=\"(\d+)\.(\d+)\".*", re.MULTILINE | re.DOTALL
r".*VERSION_ID=\"(\d+\.\d+)\".*", re.MULTILINE | re.DOTALL
)


Expand Down
16 changes: 15 additions & 1 deletion selftests/unit/utils/software_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,26 @@ def apt_supported_distro():
return distro.detect().name in ["debian", "Ubuntu"]


def login_binary_path(distro_name, distro_version):
"""Retrieve the login binary path based on the distro version""""
if distro_name == "Ubuntu":
if float(distro_version) >= 24.04:
return "/usr/bin/login"
if distro_name == "debian":
if distro_version == "trixie":
Copy link
Contributor

Choose a reason for hiding this comment

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

This ties into my previous point. It's very probably that unstable also has the file at the same place as testing, right? In the future, when those are released, it'd make sense to do a numerical version comparison IMO.

return "/usr/bin/login"
return "/bin/login"


@unittest.skipUnless(os.getuid() == 0, "This test requires root privileges")
@unittest.skipUnless(apt_supported_distro(), "Unsupported distro")
class Apt(unittest.TestCase):
def test_provides(self):
sm = manager.SoftwareManager()
self.assertEqual(sm.provides("/bin/login"), "login")
distro_name = distro.detect().name
Copy link
Contributor

Choose a reason for hiding this comment

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

And optimization here to avoid calling the detection twice:

detected = distro.detect()
login_path = login_binary_path(detected.name, detected.version)

Also, it would be OK to change login_binary_path to take a Distro instance IMO.

distro_version = distro.detect().version
login_path = login_binary_path(distro_name, distro_version)
self.assertEqual(sm.provides(login_path), "login")
self.assertTrue(isinstance(sm.backend, backends.apt.AptBackend))


Expand Down
Loading