Skip to content

Enhancement and fix for containerized pytest#3214

Merged
ptoscano merged 2 commits intomainfrom
mhorky/pytest-two-fixes
Mar 1, 2023
Merged

Enhancement and fix for containerized pytest#3214
ptoscano merged 2 commits intomainfrom
mhorky/pytest-two-fixes

Conversation

@m-horky
Copy link
Contributor

@m-horky m-horky commented Feb 23, 2023

First commit addresses one test that was being skipped on Fedora images. By including dnf-plugins-core package there as well, we can run it.

Second commit removes the excluded tests on CentOS 9 Stream, as https://bugzilla.redhat.com/show_bug.cgi?id=2167468 is now fixed.

When the package is not available, one test from
test/test_dnf_content_plugin.py is skipped.
@cnsnyder cnsnyder requested review from a team and DuckBoss and removed request for a team February 23, 2023 11:54
@github-actions
Copy link

github-actions bot commented Feb 23, 2023

Coverage

Coverage (computed on Fedora latest) •
FileStmtsMissCoverMissing
TOTAL18065440575% 
report-only-changed-files is enabled. No files were changed during this commit :)

Tests Skipped Failures Errors Time
2620 5 💤 0 ❌ 0 🔥 55.958s ⏱️

@m-horky m-horky force-pushed the mhorky/pytest-two-fixes branch from dbd8791 to ce02424 Compare February 23, 2023 15:59
Copy link
Contributor

@ptoscano ptoscano left a comment

Choose a reason for hiding this comment

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

Second commit is a fix for CentOS Stream images. Due to recent changes, podman on Fedora exposes local subscription-manager configuration in the container in the /run/secrets/ directory, making the container detection trigger.

For more context: we debugged this offline, and this was due to the system running the containers having actual files for /etc/pki/entitlement, /etc/yum.repos.d/redhat.repo, and /etc/rhsm, which podman mounts automatically on Fedora because of the symlinks in /usr/share/rhel/secrets/.

Also, the changes in certdirectory.py need a bit more thought, especially because there are many other files/modules that have the same pattern.

In the test suite, SubManFixture already takes care of patching rhsm.config.in_container, which covers the vast majority of the tests. I think it'd be better to do the same patching also in tests that do not use SubManFixture.

The other two commits are OK.

@m-horky
Copy link
Contributor Author

m-horky commented Feb 27, 2023

It is true that SubManFixture patches the in_container detection. However, because the modules are imported before any fixture is invoked, the Config object is already created without the patched function, being created in module scope. I don't see a way to fix this other than how I've done it -- we can only embrace it and make the whole codebase behave like this, possibly using Singletons.

For cases like this, where the objects needs some kind of initialization arguments, we'd need to define a place where they are initiated -- essentially, an equivalent of injectioninit.py, but for singletons. But I feel that is out of the scope. We can either merge this now as-is and start preparing proper solution, or omit the patch and only include the other two commits.

Copy link
Contributor

@ptoscano ptoscano left a comment

Choose a reason for hiding this comment

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

For cases like this, where the objects needs some kind of initialization arguments, we'd need to define a place where they are initiated -- essentially, an equivalent of injectioninit.py, but for singletons.

Sounds like our @singleton decorator, but using a small "wrapper singleton" to hold the actual object rather than making the object directly as singleton.

But I feel that is out of the scope. We can either merge this now as-is and start preparing proper solution, or omit the patch and only include the other two commits.

Yeah, this starts to get messy, and will definitely require more thoughts; also, there are more cases of config.Config() objects created as module-level variables, so changing only one makes things inconsistent (well they are already, it would grow the entropy).

Since the case it was supposed to fix is a corner one (running the tests in a c9s container from a Fedora system in which there are actual files of subscription-manager), I'd say to drop that commit for now.

Thanks!

They were required because of recent bug in systemd:
- https://bugzilla.redhat.com/show_bug.cgi?id=2167468
- systemd/systemd#26366

With this fix, we can remove the test deselections.
Copy link
Contributor

@ptoscano ptoscano left a comment

Choose a reason for hiding this comment

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

Thanks!

(fedora-36 in cockpit seems to fail, however it does not matter, as it seems to be some sort of flakyness)

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.

2 participants