Add support for process functions in verdi plugin list
#4117
Conversation
sphuber
force-pushed
the
fix/4115/verdi-plugin-list-process-functions
branch
from
ec6c428
to
22ed5f6
Compare
|
@unkcpz here is another PR that could be suitable for you. There is no rush on this one. So if you are too busy now that is fine too. |
There was a problem hiding this comment.
Thanks! it is a useful feature and an elegant implementation. @sphuber,
Besides some minor request. Before I start looking into the detail of the test cases, I just wondering is there any particular reason you create a new fixture for cli testing? If so, is that all cmdline cli run test should be transformed to this strategy?
|
Why a CI test is failed here? I guess it has nothing to do with the current PR right? |
Looks like a random transient problem and should be fixed when we rerun |
The reason I added this is because I think it makes the typical CLI tests written in pytest really concise and clear. It does all the logic that the current tests typically do, such as check for exception, and get output in lines with whitespace stripped. Currently, all tests are doing all these things a bit ad-hoc and makes the code a bit inconsistent. Ideally we would convert all tests like that to use this fixture, but don't want to do it in this PR. Anyway, the majority of tests so far still use unittest instead of |
So far, only actual `Process` subclasses were supported, however, process functions can be turned into a `Process` subclass just as well. The wrapped function gets a new attribute `spec`, which when called returns the `ProcessSpec` that is dynamically built based on the function signature of the decorated function.
sphuber
force-pushed
the
fix/4115/verdi-plugin-list-process-functions
branch
from
22ed5f6
to
57a7bed
Compare
Codecov Report
@@ Coverage Diff @@
## develop #4117 +/- ##
===========================================
+ Coverage 78.74% 78.85% +0.12%
===========================================
Files 467 467
Lines 34466 34468 +2
===========================================
+ Hits 27138 27178 +40
+ Misses 7328 7290 -38
Continue to review full report at Codecov.
|
|
|
||
| def test_plugin_list_group(run_cli_command): | ||
| """Test the `verdi plugin list` command for entry point group.""" | ||
| from aiida.plugins.entry_point import ENTRY_POINT_GROUP_TO_MODULE_PATH_MAP |
There was a problem hiding this comment.
Since the MAP is imported twice, it may be better to import it once in the beginning of file.
I see, I recomend to create a issue for future improvement. And since the |
|
Thanks for the second review @unkcpz , I have updated the |
|
Thanks! @sphuber |
|
As a reviewer, I guess it is also my duty to close #4115 ? |
No, because I have marked my OP that this closes that issue and upon merging into |
|
Thanks a lot for the reviews @unkcpz |
|
Opened #4142 to address the refactoring of existing CLI tests to use the new fixture |
Fixes #4115
So far, only actual
Processsubclasses were supported, however,process functions can be turned into a
Processsubclass just as well.The wrapped function gets a new attribute
spec, which when calledreturns the
ProcessSpecthat is dynamically built based on thefunction signature of the decorated function.
P.S.: this does not yet actually include a test through
verdi plugin listfor a registered process function since we don't have one yet. However, @mbercx should be working on a PR that would add one. If that get's merged before, I can use that to add an additional test.