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

Fixture record_testsuite_property does not work with pytest-xdist #7767

Open
marincandenza opened this issue Sep 17, 2020 · 13 comments
Open
Labels
plugin: junitxml related to the junitxml builtin plugin plugin: xdist related to the xdist external plugin topic: fixtures anything involving fixtures directly or indirectly

Comments

@marincandenza
Copy link

marincandenza commented Sep 17, 2020

According to the docs it is possible to add a junit property at test suite level. This will work fine without pytest-xdist but fails when adding -n2 or similiar to pytest args.

Error description
The test suite properties ARCH, STORAGE_TYPE shown below are not contained in the junit.xml when running pytest-xdist.

Note
I read through the existing test code which does not execute pytest-xdist. Is it wanted to call pytest-xdist in this repository?

How to reproduce

# conftest.py
import pytest

@pytest.fixture(scope="session", autouse=True)
def log_global_env_facts(record_testsuite_property):
    record_testsuite_property("ARCH", "PPC")
    record_testsuite_property("STORAGE_TYPE", "CEPH")
# test_me.py
class TestMe:
    def test_foo(self):
        assert True
(venv) pip list
Package        Version
-------------- -------
apipkg         1.5
atomicwrites   1.4.0
attrs          20.2.0
colorama       0.4.3
execnet        1.7.1
iniconfig      1.0.1
more-itertools 8.5.0
packaging      20.4
pip            20.2.3
pluggy         0.13.1
py             1.9.0
pyparsing      2.4.7
pytest         6.0.2
pytest-forked  1.3.0
pytest-xdist   2.1.0
setuptools     50.3.0
six            1.15.0
toml           0.10.1
@symonk
Copy link
Member

symonk commented Sep 17, 2020

@marincandenza could you elaborate more on the problem? Xdist by default will create a session per worker, this is by design so an auto use session scopes fixture for example will execute on each of the workers, this can be confusing at first as you expect one pytest execution, however that is not the case as all workers run pytest end to end

@marincandenza
Copy link
Author

marincandenza commented Sep 18, 2020

@symonk I have edited my bug description. The xml property on testsuite level will not be logged when invoking with pytest-xdist args n2.

@Zac-HD
Copy link
Member

Zac-HD commented Sep 19, 2020

Related to #5205.

@Zac-HD Zac-HD added plugin: junitxml related to the junitxml builtin plugin plugin: xdist related to the xdist external plugin topic: fixtures anything involving fixtures directly or indirectly labels Sep 19, 2020
@nicoddemus
Copy link
Member

nicoddemus commented Sep 19, 2020

Thanks @marincandenza,

What happens is:

  1. pytest-xdist works by having the main pytest process spawning multiple workers, which do all the collection/running. The main pytest process (master) does not do any collection/test running.
  2. Each worker runs the tests (which creates fixtures as needed), and report back the results to master, which takes care of reporting: showing results in the terminal, and importantly here, writing the junitxml file.
  3. The record_testsuite_property works internally by setting the property into the xml file handler.

So the record_testsuite_property executes on the workers, but they don't generate any reports, the master node is responsible for writing the XML file. This is the reason why the properties don't show up when using pytest-xdist.

To fix this, we need to make the properties part of the TestReport, so they can be correctly transferred to the master node when using pytest-xdist.

@nicoddemus
Copy link
Member

nicoddemus commented Sep 19, 2020

Created a PR adding a warning to the docs: #7773

FTR: I played around a bit but I don't see an easy way to fix this, I pushed to https://github.com/nicoddemus/pytest/tree/xml-properties-xdist-7767.

@mganisin
Copy link

mganisin commented Sep 24, 2020

Here is a workaround that I use (dirty hack, not a solution):

def _global_property(config, name, value):
    """Temporary hack as record_global_property doesn't work with xdist"""

    from _pytest.junitxml import xml_key
    xml = config._store.get(xml_key, None)
    if xml:
        xml.add_global_property(name, value)

Obviously inspired by junitxml.py and will work as long as the interface will remain unchanged (noticed that it is "bit live" piece of code). It isn't fixture! Personally I call it from pytest_report_header, but not an issue for me, everything what should be recorded is already known and it does the job for me (until real fix). Maybe this helps to others.

It seems obvious that this doesn't solve propagation from slaves

rplevka added a commit to rplevka/robottelo that referenced this issue Nov 4, 2020
rplevka added a commit to rplevka/robottelo that referenced this issue Nov 4, 2020
rplevka added a commit to rplevka/robottelo that referenced this issue Nov 4, 2020
mshriver pushed a commit to SatelliteQE/robottelo that referenced this issue Nov 4, 2020
rplevka added a commit to rplevka/robottelo that referenced this issue Nov 12, 2020
lpramuk pushed a commit to SatelliteQE/robottelo that referenced this issue Nov 12, 2020
@sjalloq
Copy link

sjalloq commented Feb 18, 2022

Is there any planned work to fix this? I was looking for a way to add user info to the junitxml output.

@nicoddemus
Copy link
Member

nicoddemus commented Feb 18, 2022

Hi @sjalloq, AFAIK nobody is working on it.

@sjalloq
Copy link

sjalloq commented Feb 18, 2022

@nicoddemus Thanks. Is it a fixable issue? I.E. is it worth me trying to understand the code base and look at it or is it not worth it? I tried @mganisin's hack and that seems to work okay.

@nicoddemus
Copy link
Member

nicoddemus commented Feb 18, 2022

Hi @sjalloq, TBH I don't remember the details, but it seems like it isn't trivial/obvious to fix however: #7767 (comment)

@brandonchinn178
Copy link
Contributor

brandonchinn178 commented Mar 15, 2022

To add context, this is an issue when using hypothesis with pytest-xdist with junitxml (#5202, HypothesisWorks/hypothesis#1935)

@brandonchinn178
Copy link
Contributor

brandonchinn178 commented Mar 29, 2022

So it turns out the Hypothesis pytest plugin actually didn't work with --junit-xml, regardless of whether pytest-xdist is enabled (PR: HypothesisWorks/hypothesis#3277). I got it working without pytest-xdist, and now it's running into the same issue as this ticket. The workaround mentioned by @mganisin seems to not be necessary anymore; record_testsuite_property now does the exact same thing.

xml = request.config.stash.get(xml_key, None)
if xml is not None:
record_func = xml.add_global_property # noqa
return record_func

The remaining issue with pytest-xdist is that record_testsuite_property is now a noop with pytest-xdist, since xml_key isn't in config.stash when pytest-xdist is on:

if xmlpath and not hasattr(config, "workerinput"):
junit_family = config.getini("junit_family")
config.stash[xml_key] = LogXML(
xmlpath,
config.option.junitprefix,
config.getini("junit_suite_name"),
config.getini("junit_logging"),
config.getini("junit_duration_report"),
junit_family,
config.getini("junit_log_passing_tests"),
)
config.pluginmanager.register(config.stash[xml_key])

Which goes back to this comment:

So the record_testsuite_property executes on the workers, but they don't generate any reports, the master node is responsible for writing the XML file. This is the reason why the properties don't show up when using pytest-xdist.

To fix this, we need to make the properties part of the TestReport, so they can be correctly transferred to the master node when using pytest-xdist.

@mganisin
Copy link

mganisin commented Aug 1, 2022

Thinking about this in bit more depth, "expected" behavior shouldn't be expected probably. Having record_testsuite_property as fixture means that it is ran n-times with xdist and that's unlikely desired behavior.

Maybe it could be better if there was a hook provided by junitxml to solve this (with current fixture kept for compatibility and cases without xdist).

Probably even better would be to solely depend on pytest-metadata when recording custom properties and let either junitxml or pytest-metadata (this tries to do it) to record metadata to junit output (with this for example same values will be recorded in junit as well as in pytest-html -> a bit of consistency). Actually this is a practice that I am going to start following since now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin: junitxml related to the junitxml builtin plugin plugin: xdist related to the xdist external plugin topic: fixtures anything involving fixtures directly or indirectly
Projects
None yet
Development

No branches or pull requests

7 participants