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

plugin metadata loader with pytest and bash example #136

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jscotka
Copy link
Collaborator

@jscotka jscotka commented Apr 27, 2021

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:

  • able to add additional metadata directly via python decorators
  • mixing of FMF and plugin metadata together
  • transformation of pytest marks to FMF metadata
    • mark.skip transformed to enabled: False
    • custom marks are transformed to tags
  • Added .fmf/config sa way how to configure plugins

output after cd tests/tests_plugin/:

$ PLUGINS=../../fmf/plugins/pytest.py,../../fmf/plugins/bash.py  fmf show

/pure_fmf
author: Jan Scotka <[email protected]>
summary: Pure FMF test case
test: ./runtest.sh

/runtest
author: Jan Scotka <[email protected]>
summary:  Basic test case: 1
tag:  tier1
test: ./runtest.sh

/test_basic/TestCls/test
author: Jan Scotka <[email protected]>
summary: test_basic.py TestCls test
test: python3 -m pytest -m '' -v test_basic.py::TestCls::test

/test_basic/test_fail
author: Jan Scotka <[email protected]>
summary: test_basic.py test_fail
test: python3 -m pytest -m '' -v test_basic.py::test_fail

/test_basic/test_parametrize[a]
author: Jan Scotka <[email protected]>
summary: test_basic.py test_parametrize[a]
test: python3 -m pytest -m '' -v 'test_basic.py::test_parametrize[a]'

/test_basic/test_parametrize[b]
author: Jan Scotka <[email protected]>
summary: test_basic.py test_parametrize[a]
test: python3 -m pytest -m '' -v 'test_basic.py::test_parametrize[b]'

/test_basic/test_parametrize[c]
author: Jan Scotka <[email protected]>
summary: test_basic.py test_parametrize[a]
test: python3 -m pytest -m '' -v 'test_basic.py::test_parametrize[c]'

/test_basic/test_pass
author: Jan Scotka <[email protected]>
summary: This is basic testcase
tag: Tier1
test: python3 -m pytest -m '' -v test_basic.py::test_pass

/test_basic/test_skip
author: Jan Scotka <[email protected]>
enabled: False
summary: test_basic.py test_skip
test: python3 -m pytest -m '' -v test_basic.py::test_skip

TODOs

  • probably extension is not enoung, could be regexp instead of ".py" could be e.g. ".*tests/.*\py"
  • how to behave if no data found, maybe return None and it will behave as tree node creating is skipped. (None != {} in case {} create the Node)
  • what does with modification? not easy to write data back to non fmf files, e.g. pytest tests. e.g. adding extra-nitrate key
    • solution could be add another required method for plugin to write data back for each plugin. Could be harder to parse python code and e.g. add decorator to method. if not exists (but some magic with inspect and looking into function definition source code lines and put there another line with decorator may work well and not so hard to implement inspect.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)])
    • Or store owerwritten data to separate FMF file using merging+scattering feature, could be easy. (But sorting and file priorities could cause issues here)

EDIT

  • Implemented how to modify items for plugin (pytest plugin example for new items)

Focus items for review:

  • generic config file inside .fmf dir for fmf (now used just for plugin loading)
  • changed sorting of files order `(main.fmf, ..., somename.*, somename.fmf (fmf as last in case of same names), ... )
  • modification overriding to FMF files when plugin does not support write method
  • loading plugins by file path or via python way via modules like a.b.c
  • using env variable to load (override plugins) PLUGINS env var (could be renamed to whatever)
  • where to store plugins (some could be part of FMF project) or strictly as separated in project? (then TMT needs to have some possibility to install some pip module before runnig discover on target system or part of target repo)
  • what about extension of plugin by another config (originally I have way in my pytest plugin to use own config, to be able to tweak output format, e.g. say that every TMT.tag will be translaated to tag+) e.g. I'm configuring test: item via this in various ways (another for downstream and upstream)

@lgtm-com
Copy link

lgtm-com bot commented Apr 27, 2021

This pull request introduces 2 alerts when merging 6f0291c into c08e904 - view on LGTM.com

new alerts:

  • 1 for NotImplemented is not an Exception
  • 1 for Nested loops with same variable

@jscotka jscotka force-pushed the py_plugin branch 3 times, most recently from 4b87247 to 71f3e44 Compare April 28, 2021 06:52
@lgtm-com
Copy link

lgtm-com bot commented Apr 28, 2021

This pull request introduces 2 alerts when merging 71f3e44 into c08e904 - view on LGTM.com

new alerts:

  • 1 for NotImplemented is not an Exception
  • 1 for Nested loops with same variable

@lgtm-com
Copy link

lgtm-com bot commented Apr 28, 2021

This pull request introduces 2 alerts when merging 521c0ba into c08e904 - view on LGTM.com

new alerts:

  • 1 for NotImplemented is not an Exception
  • 1 for Nested loops with same variable

@jscotka jscotka requested review from psss and lukaszachy April 28, 2021 07:49
@jscotka jscotka added discuss enhancement New feature or request labels Apr 28, 2021
@jscotka
Copy link
Collaborator Author

jscotka commented Apr 28, 2021

added to hacking session as topic.

@lgtm-com
Copy link

lgtm-com bot commented Apr 28, 2021

This pull request introduces 2 alerts when merging 9aa90b8 into c08e904 - view on LGTM.com

new alerts:

  • 1 for NotImplemented is not an Exception
  • 1 for Nested loops with same variable

@psss psss marked this pull request as draft April 28, 2021 10:19
@jscotka jscotka changed the title pytest plugin metadata loader plugin metadata loader with pytest and bash example Apr 28, 2021
@lgtm-com
Copy link

lgtm-com bot commented Apr 29, 2021

This pull request introduces 3 alerts when merging 94b77e4 into c08e904 - view on LGTM.com

new alerts:

  • 2 for NotImplemented is not an Exception
  • 1 for Nested loops with same variable

@jscotka jscotka force-pushed the py_plugin branch 3 times, most recently from c0f184a to 46f442a Compare May 4, 2021 10:27
@lgtm-com
Copy link

lgtm-com bot commented May 24, 2021

This pull request introduces 4 alerts when merging 5ae40a7 into 6ffce89 - view on LGTM.com

new alerts:

  • 3 for Unused import
  • 1 for Incomplete ordering

@lgtm-com
Copy link

lgtm-com bot commented May 25, 2021

This pull request introduces 2 alerts and fixes 1 when merging 4734429 into 6ffce89 - view on LGTM.com

new alerts:

  • 1 for Unused import
  • 1 for Incomplete ordering

fixed alerts:

  • 1 for Unused import

@jscotka jscotka force-pushed the py_plugin branch 4 times, most recently from 6383667 to 42b4965 Compare May 25, 2021 13:45
@lgtm-com
Copy link

lgtm-com bot commented May 25, 2021

This pull request fixes 1 alert when merging e5912aa into 6ffce89 - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented May 25, 2021

This pull request fixes 1 alert when merging 244c828 into 6ffce89 - view on LGTM.com

fixed alerts:

  • 1 for Unused import

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
@lgtm-com
Copy link

lgtm-com bot commented May 26, 2021

This pull request fixes 1 alert when merging 3d1330b into 6ffce89 - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented May 26, 2021

This pull request fixes 1 alert when merging e913936 into 6ffce89 - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@jscotka jscotka marked this pull request as ready for review May 26, 2021 09:32
import inspect
import os
import re
from functools import lru_cache
Copy link
Collaborator

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.

Copy link
Collaborator

@lukaszachy lukaszachy left a 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
Copy link
Collaborator

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.

Copy link
Collaborator Author

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"
Copy link
Collaborator

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

Copy link
Collaborator Author

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}" """
Copy link
Collaborator

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.

Copy link
Collaborator Author

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
Copy link
Collaborator

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

Copy link
Collaborator Author

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.

@jscotka
Copy link
Collaborator Author

jscotka commented May 26, 2021

/packit test

return cls._set_fn(name)


class TMT(metaclass=__TMTMeta):
Copy link
Collaborator

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

Copy link
Collaborator Author

@jscotka jscotka Jul 3, 2023

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

see code https://github.com/teemtee/fmf/pull/136/files#diff-90dd8e8769d61574c4e3b75280a1a7d8425336e9ab8ec30b4ef71543d44f9120R72

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants