-
Notifications
You must be signed in to change notification settings - Fork 224
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
Tests: Add the EntryPointManager
exposed as entry_points
fixture
#5656
Tests: Add the EntryPointManager
exposed as entry_points
fixture
#5656
Conversation
bbb8ede
to
8ac7b9a
Compare
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.
I have to admit this implementation is a bit fancy for me. I have two questions before I dive in futher.
def test_entry_points_add_invalid(entry_points): | ||
"""Test the :meth:`EntryPointManager.add` method.""" | ||
with pytest.raises(TypeError, match='`entry_point_string` should be a string when defined.'): | ||
entry_points.add('some.module:SomeClass', []) |
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.
After adding to the entry point, should it be cleaned after the test?
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.
In this case it wouldn't even be added, because the add call is invalid and an exception is thrown. But even if it would have been valid and an entry point would be added, the cleanup is done automatically by the fixture. The actual entrypoints are monkeypatched by the fixture, so by the time the fixture goes out of scope (i.e. after the tests) the original entry points are simply restored.
da63a8b
to
4f712b4
Compare
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.
Hi @sphuber, just some minor things, all others look good. But I only reviewed the code itself, I don't have too much idea how it should work for the process monitor.
return module_entry_point.eps() | ||
|
||
@staticmethod | ||
def _validate_entry_point(entry_point_string: str | None, group: str | None, name: str | None) -> tuple[str, str]: |
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.
Is entry_point_string
exclusive with group
and name
, I guess only entry_point_string
should be enough?
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.
Indeed, you only need either entry_point_string
, or both group
and name
. Sometimes it is more convenient to pass the group
and name
separately and not have to do something like f{group}:{name}
yourself.
self, | ||
value: type | str, | ||
entry_point_string: str | None = None, | ||
*, |
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.
What is this for?
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.
The *
operator signifies that all arguments after that are keyword-only arguments, i.e., they can only be specified using keywords. See this PEP. This is useful for this interface where group
and name
are mutually exclusive with entry_point_string
. With this syntax a user calls either
entry_points.add('aiida.parsers:core.base')
or
entry_points.add(group='aiida.parsers', name='core.base')
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.
I see, thanks! Then maybe also use this for _validate_entry_point
for the exclusive reason I mentioned above?
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.
The _validate_entry_point
is just an internal private method to perform the validation. That is also the reason I didn't use the *
method, since here it is not really necessary since only the class itself is calling this. No real point then to add it here.
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.
That makes sense. 👍🏻
This fixture allows to easily add or remove entry points for the scope of the function. It temporarily manipulates the content of the entry point cache as provided by `importlib.metadata.entry_points`. After the test function ends, the additions and removals are undone. The `SelectableEntryPoints` class fully implements the dictionary interface which we use to update its state, however, this interface is deprecated. The methods that replace it though, the `select` method, are read-only and do not allow to modify the state, so we cannot use them. To not flood the logs with deprecation warnings, they are intentionally suppressed.
4f712b4
to
338d50d
Compare
You can see an example of how I use it in the tests of the You can see that I define a monitor just above the test, and then use the |
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.
Thanks for the link to the use case, that makes more clear to me.
If you can check and answer the comment about using *
in _validate_entry_point
, then I think it is all good.
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.
🚀
Fixes #5655
This fixture allows to easily add or remove entry points for the scope
of the function. It temporarily manipulates the content of the entry
point cache as provided by
importlib.metadata.entry_points
. After thetest function ends, the additions and removals are undone.
The
SelectableEntryPoints
class fully implements the dictionaryinterface which we use to update its state, however, this interface is
deprecated. The methods that replace it though, the
select
method,are read-only and do not allow to modify the state, so we cannot use
them. To not flood the logs with deprecation warnings, they are
intentionally suppressed.