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

Tests: Add the EntryPointManager exposed as entry_points fixture #5656

Merged
merged 1 commit into from
Sep 28, 2022

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Sep 22, 2022

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 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.

@sphuber sphuber force-pushed the feature/5655/entry-points-fixture branch 2 times, most recently from bbb8ede to 8ac7b9a Compare September 28, 2022 09:10
Copy link
Member

@unkcpz unkcpz left a 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.

tests/conftest.py Show resolved Hide resolved
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', [])
Copy link
Member

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?

Copy link
Contributor Author

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.

@sphuber sphuber force-pushed the feature/5655/entry-points-fixture branch 2 times, most recently from da63a8b to 4f712b4 Compare September 28, 2022 12:14
@sphuber sphuber requested a review from unkcpz September 28, 2022 12:43
Copy link
Member

@unkcpz unkcpz left a 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.

tests/conftest.py Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
return module_entry_point.eps()

@staticmethod
def _validate_entry_point(entry_point_string: str | None, group: str | None, name: str | None) -> tuple[str, str]:
Copy link
Member

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?

Copy link
Contributor Author

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,
*,
Copy link
Member

Choose a reason for hiding this comment

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

What is this for?

Copy link
Contributor Author

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')

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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.
@sphuber sphuber force-pushed the feature/5655/entry-points-fixture branch from 4f712b4 to 338d50d Compare September 28, 2022 13:56
@sphuber sphuber requested a review from unkcpz September 28, 2022 13:56
@sphuber
Copy link
Contributor Author

sphuber commented Sep 28, 2022

But I only reviewed the code itself, I don't have too much idea how it should work for the process monitor.

You can see an example of how I use it in the tests of the CalcJob monitor PR here: https://github.com/sphuber/aiida-core/blob/f20bb132a9303dcec6ca238c5e41ec1eb3273d1d/tests/engine/processes/calcjobs/test_calc_job.py#L1037

You can see that I define a monitor just above the test, and then use the entry_points fixture to quickly register it through an entry point, so I can use that entry point straight after in the test itself. Normally you would have to do some hacking with monkeypatch replacing an existing entry point, but I feel this is way cleaner and can even be used in situations where no actual entry point exist to be monkeypatched, making it more versatile

Copy link
Member

@unkcpz unkcpz left a 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.

@sphuber sphuber requested a review from unkcpz September 28, 2022 14:42
Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

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

🚀

@sphuber sphuber merged commit 8fe76db into aiidateam:main Sep 28, 2022
@sphuber sphuber deleted the feature/5655/entry-points-fixture branch September 28, 2022 14:55
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.

Add fixture that makes it easy to add entry points for unit tests
2 participants