Fix inheritance issues and separate the mocked dbus into an object #184
Conversation
This file contains 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
whot
commented
Nov 6, 2023
whot
added a commit
to whot/xdg-desktop-portal
that referenced
this pull request
Nov 6, 2023
For historical reasons, dbusmock is written for Python's unittest framework and most of what it does is done via @classmethod. We called it on the subclassed object though, so we got some weird behaviors. In particular, things like `a = foo.someval` would resolve to the parent class' `someval` but `foo.someval = a` would set that value on the subclassed object. This lead, indirectly, to our forked dbus-daemon not getting terminated. This needs to be be fixed in dbusmock (see [1]) but for now simply work around this by instantiating the DBusTestCase class directly. [1] martinpitt/python-dbusmock#184
|
ftr I'm also planning to move with spawn_server() as blah:
.... and have it auto-terminate instead of requiring callers to track the Popen objects. |
whot
added a commit
to whot/xdg-desktop-portal
that referenced
this pull request
Nov 6, 2023
For historical reasons, dbusmock is written for Python's unittest framework and most of what it does is done via @classmethod. We called it on the subclassed object though, so we got some weird behaviors. In particular, things like `a = foo.someval` would resolve to the parent class' `someval` but `foo.someval = a` would set that value on the subclassed object. This lead, indirectly, to our forked dbus-daemon not getting terminated. This needs to be be fixed in dbusmock (see [1]) but for now simply work around this by instantiating the DBusTestCase class directly. [1] martinpitt/python-dbusmock#184
github-merge-queue bot
pushed a commit
to flatpak/xdg-desktop-portal
that referenced
this pull request
Nov 6, 2023
For historical reasons, dbusmock is written for Python's unittest framework and most of what it does is done via @classmethod. We called it on the subclassed object though, so we got some weird behaviors. In particular, things like `a = foo.someval` would resolve to the parent class' `someval` but `foo.someval = a` would set that value on the subclassed object. This lead, indirectly, to our forked dbus-daemon not getting terminated. This needs to be be fixed in dbusmock (see [1]) but for now simply work around this by instantiating the DBusTestCase class directly. [1] martinpitt/python-dbusmock#184
Merged
martinpitt
requested changes
Nov 10, 2023
There was a problem hiding this comment.
Thanks @whot for your work here! I like this. It's still draft and there are several test failures, but I did a first round of review to get some feedback.
Sorry about the conflict -- I just pushed a branch to fix the CI regression on main, which happened because fedora:latest moved from F38 to F39.
whot
commented
Nov 10, 2023
whot
force-pushed
the
wip/fix-inheritance-issues
branch
2 times, most recently
from
82606cf to
5c4242c
Compare
|
Updated, with the main change aside from the various fixes being that there's now a |
We probably shouldn't set it to None before checking whether it's None, that seems less efficient than it could be.
On the run of a derived test class, cls._DBusTestCase__datadir calls up into the parent class for the datadir and removes it. The subsequent statement cls._DBusTestCase__datadir = None sets it on the derived class, not on DBusTestCase which remains at the original value. Doesn't matter too much since the next derived test class will repeat that process but we leave the parent datadir set despite having removed it which seems like a bad idea. See the comment in get_services_dir(), the datadir is always on the DBusTestCase class, so let's mirror that behavior here.
A slightly weird side-effect here: start_dbus was called on the
DBusTestCase class - thus the session/system bus pid was set on that
class.
But tearDownClass() was called with the derived class from the test, so
while the getters for variables resolved correctly,
setattr(cls, f"{bus_type}_bus_pid", None)
would set TestFoo.session_bus_pid = None and leave
DBusTestCase.session_bus_pid at whatever value it had before.
Fix this and add an assert to ensure we never start a session/system bus
twice in the same test class.
whot
force-pushed
the
wip/fix-inheritance-issues
branch
8 times, most recently
from
5b08061 to
e54a23c
Compare
No real functional changes, this prepares the way for some extra asserts in the future and the behavior matches that of unittest-style classes which are always subclasses of DBusTestCase.
This is a potential break but it's unlikely to affect any old-style unittest classes since they would be subclasses by default. It may affect new-style pytest classes, if any exist, but OTOH it can catch confusing errors like setting a variable on the DBusTestCase class but then unsetting it on the derived class during a test.
This is what they are anyway, so let's do this and avoid the possibly accidental wrong class handling fixed a few commits ago.
This patch introduces the BusType class which handles the system/session
buss differences including whatever is independent of who started the
actual daemon such as getting a connection.
It moves spawn_server and spawn_server_template to standalone functions
since those do not have any dependency on anything else in
DBusTestCase.
It adds the PrivateDBus class which wraps a single instance of a DBus
daemon, with a context manager so we can do things like:
with PrivateDBus(BusType.SESSION) as bus:
....
and have it self clean up automatically. Unlike the previous approach,
we don't just fork in the background and kill via PID, we keep a
reference to the process and kill it via Python's API.
Finally, it changes the DBusTestCase class to use a single instance of
those objects as its DBus server when created with start_session_bus()
or start_system_bus(). Most of that class is now a wrapper around the
PrivateDBus object (or the BusType object where appropriate).
PrivateDBus also wraps some functions to make it look more like a
DBusTestCase object to keep compatibility with the current pytest
behavior.
With all that, we can now return a PrivateDBus object from the pytest
fixtures and have better insulation than before where the DBusTestCase
class was responsible for singleton handling.
whot
force-pushed
the
wip/fix-inheritance-issues
branch
from
e54a23c to
6b51c9e
Compare
Let's move spawn_server and spawn_server_template into an object with a context manager, greatly simplifying them where fixtures are being used. Note that both are renamed into: SpawnedMock.spawn_for_name() SpawedMock.spawn_with_template()
Some of these could've only been exposed by the recently added pytest fixtures - those have never been in a release so let's drop them.
deciseconds is a rather unusual unit. Let's switch the real API we now have to use seconds: float like time.sleep() and wrap our old API around that.
whot
force-pushed
the
wip/fix-inheritance-issues
branch
from
6b51c9e to
c39fc5b
Compare
|
Ok, pipeline is green and I think this is ready. Or at least I've rebased and stared at it enough that I no longer see any bugs... |
martinpitt
approved these changes
Nov 30, 2023
|
@whot Do you want this in a release now, or do you have some more things you want to work on before that? |
|
I have nothing else in the pipe right now (beyond whatever bugs I just introduced in this MR ;) Thanks heaps for the quick merge! |
|
Ack, here is https://github.com/martinpitt/python-dbusmock/releases/tag/0.30.0 . Uploaded to Debian, and Fedora updates are in progress. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Filing as draft for now because the last commit may break backwards compat, see below.
I noticed that my dbus-daemons didn't get terminated when using dbusmock in the xdg-desktop-portal which led to a bit of a rabbit hole. So I figured "let's fix this properly" 😄
For (understandable) historical reasons, the
DBusTestCaseclass handled everything as@classmethod. That causes a few issues though because theclsargument is mostly the real class but in some cases it'sDBusTestCase. This can lead to a getter resolving toDBusTestCase.somevarbut the setter resolving toMyTestClass(DBusTestCase).somevar.The first few patches disentangle that to make sure we're always on the correct class instead of oscillating between
DBusTestCaseand the test class.The last (and most interesting) patch is to add a new
MockedDBusobject that wraps the actually started daemon.I've patched this so
DBusTestCasewraps this new class but this may introduce some compatibility issues. Arguably we could leaveDBusTestCaseuntouched and as legacy API and suggest usingMockedDBusvia pytest for the future. Since the pytests haven't been in a release yet we could also remove some of the wrappers from this new class and rely on callers to use the newBusType/spawn_*API directly.Ironically, the last patch somewhat undoes the previous patches in that everything is now on the
DBusTestCaseclass, but that shouldn't matter much.It does pass the test suite here, so any incompatibilities introduced are not tested ;)