-
Notifications
You must be signed in to change notification settings - Fork 28
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
plugin metadata loader with pytest and bash example #136
base: main
Are you sure you want to change the base?
Conversation
This pull request introduces 2 alerts when merging 6f0291c into c08e904 - view on LGTM.com new alerts:
|
4b87247
to
71f3e44
Compare
This pull request introduces 2 alerts when merging 71f3e44 into c08e904 - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging 521c0ba into c08e904 - view on LGTM.com new alerts:
|
added to hacking session as topic. |
This pull request introduces 2 alerts when merging 9aa90b8 into c08e904 - view on LGTM.com new alerts:
|
This pull request introduces 3 alerts when merging 94b77e4 into c08e904 - view on LGTM.com new alerts:
|
c0f184a
to
46f442a
Compare
This pull request introduces 4 alerts when merging 5ae40a7 into 6ffce89 - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts and fixes 1 when merging 4734429 into 6ffce89 - view on LGTM.com new alerts:
fixed alerts:
|
6383667
to
42b4965
Compare
This pull request fixes 1 alert when merging e5912aa into 6ffce89 - view on LGTM.com fixed alerts:
|
This pull request fixes 1 alert when merging 244c828 into 6ffce89 - view on LGTM.com fixed alerts:
|
Trasnfer code from fmf_metadata directly to FMF add there important improvement of file nadling Use cnfig inside .fmf dir to load plugins adjust tests and skip data files
This pull request fixes 1 alert when merging 3d1330b into 6ffce89 - view on LGTM.com fixed alerts:
|
This pull request fixes 1 alert when merging e913936 into 6ffce89 - view on LGTM.com fixed alerts:
|
import inspect | ||
import os | ||
import re | ||
from functools import lru_cache |
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.
fmf should support also python2.7 where this is not available.
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.
Few comments for now, I need to understand the code for the rest :)
Also an exception when PLUGINS contains broken path could be better.
@lru_cache(maxsize=None) | ||
def enabled_plugins(*plugins): | ||
plugins = os.getenv(PLUGIN_ENV).split( | ||
",") if os.getenv(PLUGIN_ENV) else plugins |
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 believe TMT splits on space, let's be consistent.
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.
Yes, I can change it to whatever more consistent solution, I also thought to use :
as std path separator, or
but
is worse thanks to it is more common to have spaces in paths, than :
or ,
chars
MAIN = "main" + SUFFIX | ||
IGNORED_DIRECTORIES = ['/dev', '/proc', '/sys'] | ||
# comma separated list for plugin env var | ||
PLUGIN_ENV = "PLUGINS" |
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.
FMF_PLUGINS
to mitigate name conflict
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.
Yes, sounds reasonable.
"test": """ | ||
cls_str = ("::" + str(cls.name)) if cls.name else "" | ||
escaped = shlex.quote(filename + cls_str + "::" + test.name) | ||
f"python3 -m pytest -m '' -v {escaped}" """ |
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'm not sure about pytest as a require :/ As it is now it is a 'hidden' require - you need to remember. But on the other hand it leaves up to user whether rpm or pip install is better.
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.
yes, it is now hidden, you are rigt, and theoretically it is also why I've let it configurable, not hardcoded. in my project fmf_metadata
I have own configs for upstream and dowstream data generation. and in case you have just unittests then you an execute it directly via python unittest
executor, or via some nose
so that pytest is just default way.
if key == "skip": | ||
TMT.enabled(False)(func) | ||
elif key == "skipif": | ||
# add skipif as tag as well (possible to use adjust, but |
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.
Could skipif be evaluated at this point? Imho mark.skipif(False)
should end up as enabled: False
and not tag: SKIP False
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.
It is not possible, because evaluation will be done in sense of TMT on guest
, although TMT discovery is done on host
system.
This skipIf
is little bit problem to incorporate somehow easily into FMF metadata, but this tag was the simplest solution and probably the easy to understand.
Probably the beast one could be to trasfer it to something like:
adjust:
when:
interpreter: python
code: False
enabled: False
and execute adjust condition on guest system. But nothing similar is supported right now.
/packit test |
return cls._set_fn(name) | ||
|
||
|
||
class TMT(metaclass=__TMTMeta): |
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.
One thing I can say I disagree with right away: this approach taints fmf with a custom code to make a single library user happy and makes fmf do things that are IMO well beyond its scope.
First, it makes adding and changing tmt specifications harder, adding a new place that needs to be updated when specification changes. But, more importantly: I don't think it's up to fmf to inspect the input or validate it in any way.
Is fmf validating or converting tmt keys when reading them from a genuine fmf file? I don't think that's the case, and it should not happen here either. If I put tag: false
into my main.fmf
of a test, fmf library will not complain, it will not touch the value, it will not check against a tmt specification schema, it will read it, put it into a fmf tree, and it will deliver the key and its value to tmt. End of story. It's tmt who's responsible for validating the input and reporting its type is invalid and unsupported.
Would it be possible to make the pytest plugin as "dumb" as the actual fmf processing, i.e. treating the input values as opaque blobs of key & value bits, and stacking them into a final fmf tree? I bet it would be possible with some clever Python magic, either have an open decorator accepting all kinds of kwargs or some dynamic decorators - but I strongly believe the openness here is a good thing, not a bug - fmf should not care about key names or value types, it does not care about them when reading them from *.fmf
files.
# Probably the simples, but less readable:
@fmf(
tag=['foo', 'bar'],
adjust=[
{...}
...)
# Or maybe allow YAML-in-a-string and parse it as if read from an fmf file?
@fmf(
"""
author: [email protected]
tag:
- foo
- bar
adjust:
...
""")
# Or with some __getattr__ magic, this could be possible by creating dynamic decorators aware of their name, i.e. the key:
@fmf.author('[email protected]')
@fmf.tag(['foo', 'bar'])
...
tmt is and should remain the sole owner of the semantics. Just like any other tool using fmf to read data. It's not up to fmf to judge, nobody asked fmf's opinion on keys from *.fmf
files, why would it matter here?
Should a competing test case management tool appear, build on top of fmf too, but with a different set of keys, would fmf learn its semantics as well? And when another two appear, these brand new and cool mmt
and ttm
tools, will fmf again learn about their semantics? I don't think that's a good road to take :)
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.
you are right, semantics, could be part of another tool. I've Implemented it as one module, because in this case it is all in one solution. I've wanted to show complete solution. My idea was that in FMF there will be just support for plugin, and implementation of plugins will be separate project or part of TMT. But plugin solution alone is hard to test itself. so that complet solution also proves how it works and it also show usecase in TMT or whatever else. I'm familiar to split it. But we have to agreed on the solution for plugin. and improve this part to have it testable as much as possible and when we found it is good enough then I'll split it.
Syntax this is exactly what is supported there it is using internal method getattibute
to handle caller name and then base on semantics it is decided whats wrong and whats implemented and well structured:
@fmf.author('[email protected]')
It is WIP, so please no stylish comments, I expect more fundamental comments of this architecture design
example of plufing for FMF to discover pytest tests and bash.
cool features:
mark.skip
transformed to enabled: False.fmf/config
sa way how to configure pluginsoutput after
cd tests/tests_plugin/
:TODOs
".*tests/.*\py"
None != {}
in case {} create the Node)extra-nitrate
keyinspect.getfile(item.function), inspect.getsourcelines(item.function)[1]
and insert new line into the file if not found inside other decorators already in: [line for line in inspect.getsourcelines(item.function)[0] if re.match(r"\s*@FMF",line)]
)EDIT
Focus items for review:
a.b.c
PLUGINS
env var (could be renamed to whatever)discover
on target system or part of target repo)TMT.tag
will be translaated totag+
) e.g. I'm configuringtest:
item via this in various ways (another for downstream and upstream)