Make record_xml_property generic and compatible with xdist and markers. #2770

carlos-jenkins · 2017-09-11T21:40:37Z

This PR changes the record_xml_property function with a more generic record_property that is compatible with xdist, markers, and any other reporter other than the JUnit reporter.

The implementation adds a list of user_properties to the item object, and can be modified anytime after its creation during the test lifecycle. When the test has finished, the user properties are passed to the TestReport object, which is shared with any reporter, and in particular serialized and sent to the master in xdist, and then the reporter are free to plot this properties as they want.

Currently, to maintain the functionality, the JUnit adds those properties to the XML report.

In addition, this allows markers to modify (add, delete) user properties as desired. This is documented in this PR and a particular use case is:

# Content of conftest.py

def pytest_collection_modifyitems(session, config, items):
    for item in items:
        marker = item.get_marker('test_id')
        if marker is not None:
            test_id = marker.args[0]
            item.user_properties.append(('test_id', test_id))

That allows to use markers as follows:

# Content of test_function.py

@pytest.mark.test_id(1501)
def test_function():
    assert True

Regards

RonnyPfannschmidt

hi Carlos,

thanks for this addition, this is a nicely done next step,

however we do have some major problems there

a) this breaks backward compatibility harshly - a removal without a deprecation period is unacceptable

a backward compatibility fixture that's just an alias triggering a deprecating warning for the node should be fine
b) this removes the warning about being experimental - which would remove a safety net that's there for our own sake

carlos-jenkins · 2017-09-11T22:11:04Z

@RonnyPfannschmidt totally reasonable. I'll add those.

Thanks for the review.

carlos-jenkins · 2017-09-11T22:30:18Z

@RonnyPfannschmidt deprecation warnings added. Back to you.

nicoddemus · 2017-09-12T01:14:36Z

As @RonnyPfannschmidt said, thanks a lot again @carlos-jenkins for your work! At first seems like a good step forward.

Some thoughts we should discuss:

Agree we should deprecate record_xml_property, thanks for tackling it.
I wonder if we need record_property at all? If we make user_properties a public attribute of Node, users can use that directly through request.node.user_properties instead. Not that it is a big deal because the fixture itself is trivial, but I still wonder if we should add a redundant API for little gain.
We should probably document what types are acceptable as name and value to be put inside user_properties. Perhaps advise that depending on what's put in there it might not work with xdist? Should we make some effort to add more concrete support for example "anything json-serializable"?

nicoddemus · 2017-11-10T21:16:55Z

@carlos-jenkins gentle ping, would like to hear if you have any thoughts about my comments.

carlos-jenkins · 2017-11-13T17:23:12Z

Hi @nicoddemus , sorry I didn't commented earlier, I had my priorities shifted and was unable to keep working on this branch. In relation to your comments:

Great.
I think there is value in keeping record_property, not only it is the main use case, it makes pytest to have "built-in bateries". Plus, many people already knows and uses record_xml_property so it offers a simple migration path.
This is interesting. I would say that name is always string, and value not sure, but at most it should be simple datatypes like str, int, float. In the XML they will be stored as strings anyway. Or maybe values can be whole dictionaries, and export to json in XML, but again, not sure :/

nicoddemus · 2017-11-13T20:28:29Z

I had my priorities shifted and was unable to keep working on this branch.

No worries, thanks for getting back to this.

I think there is value in keeping record_property, not only it is the main use case, it makes pytest to have "built-in bateries". Plus, many people already knows and uses record_xml_property so it offers a simple migration path.

Good point, I agree.

at most it should be simple datatypes like str, int, float

Fair enough, let's go with that! 👍

dajose · 2017-12-12T15:32:19Z

hey, I just solved some merge conflicts :)

is this PR ready to go? (I see the @RonnyPfannschmidt requested changes were applied)

nicoddemus · 2017-12-13T09:00:54Z

Thanks @dajose, would you mind take a look at the CI errors?

nicoddemus · 2017-12-13T09:01:42Z

Also because this is a new feature, this PR should target the features branch.

RonnyPfannschmidt · 2017-12-13T12:41:41Z

it seems doc-tests lack the feature right now

RonnyPfannschmidt · 2017-12-13T12:44:45Z

_pytest/python.py

@@ -1132,6 +1132,10 @@ def __init__(self, name, parent, args=None, config=None,
        #: .. versionadded:: 3.0
        self.originalname = originalname

+        #: user properties is a list of tuples (name, value) that holds user
+        #: defined properties for this test.
+        self.user_properties = []


we either need to add this in more places, or default to a empty list when trying to fetch it (i beleive its a good idea to move it to the item class)

Thanks for the idea :)

I just added a commit with that change

original problem was nicely handled

coveralls · 2018-01-02T17:34:39Z

Coverage decreased (-0.2%) to 92.559% when pulling beafa0a on HPENetworking:master into 5c6d773 on pytest-dev:master.

dajose · 2018-01-02T17:49:14Z

could you help me understanding the CI checks? it seems to me like there was some error unrelated with the changes

Also, @nicoddemus is this PR really a "feature"? I think it like a enhancement to support an existing functionality when using xdist.

nicoddemus · 2018-01-09T19:56:21Z

Hi @dajose and @carlos-jenkins, sorry for the delay.

Also, @nicoddemus is this PR really a "feature"? I think it like a enhancement to support an existing functionality when using xdist.

You are correct, we just use the "features" branch as the target for enhancements/features for the next minor release. This should not go into a new bugfix release (master) because we don't want users to depend on bug fix releases because of new functionality.

nicoddemus · 2018-01-09T20:00:21Z

About the checks: they seem unrelated and can be ignored.

dajose · 2018-01-09T20:02:48Z

@nicoddemus so, we close this PR and open another for branch features? (is there other way to do that?)

nicoddemus

Aside from my minor comment this looks good to go from my POV. 👍

nicoddemus · 2018-01-09T20:01:22Z

_pytest/runner.py

@@ -315,7 +316,8 @@ class TestReport(BaseReport):
    """

    def __init__(self, nodeid, location, keywords, outcome,
-                 longrepr, when, sections=(), duration=0, **extra):
+                 longrepr, when, user_properties,


I think user_properties should go after duration to avoid breaking clients needlessly.

nicoddemus · 2018-01-09T20:04:32Z

@dajose you can change the target of the branch by clicking on the Edit button next to the title of the PR.

After that it will probably be better to rebase your commits into the features branch.

coveralls · 2018-01-09T20:26:26Z

Coverage increased (+0.002%) to 92.572% when pulling 567b1ea on HPENetworking:master into 97bb6ab on pytest-dev:features.

RonnyPfannschmidt · 2018-01-17T16:27:39Z

@carlos-jenkins can you rebase so it turns merge-able again?

Ankitagupta2309 · 2018-02-16T02:31:29Z

Hi @carlos-jenkins,

This is a highly useful functionality. When can we merge this PR.
I need to use the same functionality in our unit-tests and would really appreciate if it could get merged in master.

…d logic so that the properties are passed to the TestReport object and thus allow compatibility with pytest-xdist.

carlos-jenkins · 2018-02-20T21:47:36Z

@RonnyPfannschmidt @nicoddemus rebase done. Now waiting for CI.

Also make record_xml_property return record_property directly

carlos-jenkins · 2018-02-21T20:52:46Z

@RonnyPfannschmidt @nicoddemus CI ready :D

…ibility

nicoddemus · 2018-02-21T20:59:16Z

Thanks a ton @carlos-jenkins!

I just pushed two small changes: added a note about deprecating record_xml_property and moved user_properties to the end of the parameter list in TestReport.__init__.

This is ready for merging from my POV! 👍

Starting 2018 (many pytest versions ago) the correct way to record properties is by using the "record_property" method pytest-dev/pytest#2770

carlos-jenkins force-pushed the master branch from 85e9644 to 6ca04bd Compare September 11, 2017 21:50

RonnyPfannschmidt previously requested changes Sep 11, 2017

View reviewed changes

RonnyPfannschmidt reviewed Dec 13, 2017

View reviewed changes

nicoddemus requested changes Jan 9, 2018

View reviewed changes

carlos-jenkins changed the base branch from master to features January 9, 2018 22:27

nicoddemus mentioned this pull request Jan 18, 2018

Allow recording custom xml attributes #3130

Closed

carlos-jenkins force-pushed the master branch 4 times, most recently from 453cca6 to 3377c2f Compare February 20, 2018 21:03

carlos-jenkins force-pushed the master branch 3 times, most recently from dd7120c to 4a7e234 Compare February 20, 2018 21:46

Renamed the fixture record_xml_property to record_property and adapte…

8b49ddf

…d logic so that the properties are passed to the TestReport object and thus allow compatibility with pytest-xdist.

carlos-jenkins force-pushed the master branch from 4a7e234 to 8b49ddf Compare February 20, 2018 21:46

Add note about deprecating record_xml_property

d838193

Also make record_xml_property return record_property directly

Move user_properties to the end of parameter list for backward compat…

567b1ea

…ibility

nicoddemus approved these changes Feb 21, 2018

View reviewed changes

RonnyPfannschmidt merged commit 54e63b7 into pytest-dev:features Feb 22, 2018

RonnyPfannschmidt mentioned this pull request Feb 22, 2018

record_xml_property does work in parallel execution #3252

Closed

dajose added a commit to HPENetworking/topology that referenced this pull request Jul 12, 2023

Fix recording of test id mark

167a478

Starting 2018 (many pytest versions ago) the correct way to record properties is by using the "record_property" method pytest-dev/pytest#2770

dajose mentioned this pull request Jul 12, 2023

Fix recording of test id mark HPENetworking/topology#99

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make record_xml_property generic and compatible with xdist and markers. #2770

Make record_xml_property generic and compatible with xdist and markers. #2770

carlos-jenkins commented Sep 11, 2017 •

edited

Loading

RonnyPfannschmidt left a comment

carlos-jenkins commented Sep 11, 2017

carlos-jenkins commented Sep 11, 2017

nicoddemus commented Sep 12, 2017 •

edited

Loading

nicoddemus commented Nov 10, 2017

carlos-jenkins commented Nov 13, 2017

nicoddemus commented Nov 13, 2017

dajose commented Dec 12, 2017 •

edited

Loading

nicoddemus commented Dec 13, 2017 •

edited

Loading

nicoddemus commented Dec 13, 2017

RonnyPfannschmidt commented Dec 13, 2017

RonnyPfannschmidt Dec 13, 2017

dajose Jan 2, 2018

coveralls commented Jan 2, 2018

dajose commented Jan 2, 2018 •

edited

Loading

nicoddemus commented Jan 9, 2018

nicoddemus commented Jan 9, 2018

dajose commented Jan 9, 2018

nicoddemus left a comment

nicoddemus Jan 9, 2018

nicoddemus commented Jan 9, 2018

coveralls commented Jan 9, 2018 •

edited

Loading

RonnyPfannschmidt commented Jan 17, 2018

Ankitagupta2309 commented Feb 16, 2018

carlos-jenkins commented Feb 20, 2018

carlos-jenkins commented Feb 21, 2018

nicoddemus commented Feb 21, 2018 •

edited

Loading

Make record_xml_property generic and compatible with xdist and markers. #2770

Make record_xml_property generic and compatible with xdist and markers. #2770

Conversation

carlos-jenkins commented Sep 11, 2017 • edited Loading

RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

carlos-jenkins commented Sep 11, 2017

carlos-jenkins commented Sep 11, 2017

nicoddemus commented Sep 12, 2017 • edited Loading

nicoddemus commented Nov 10, 2017

carlos-jenkins commented Nov 13, 2017

nicoddemus commented Nov 13, 2017

dajose commented Dec 12, 2017 • edited Loading

nicoddemus commented Dec 13, 2017 • edited Loading

nicoddemus commented Dec 13, 2017

RonnyPfannschmidt commented Dec 13, 2017

RonnyPfannschmidt Dec 13, 2017

Choose a reason for hiding this comment

dajose Jan 2, 2018

Choose a reason for hiding this comment

coveralls commented Jan 2, 2018

dajose commented Jan 2, 2018 • edited Loading

nicoddemus commented Jan 9, 2018

nicoddemus commented Jan 9, 2018

dajose commented Jan 9, 2018

nicoddemus left a comment

Choose a reason for hiding this comment

nicoddemus Jan 9, 2018

Choose a reason for hiding this comment

nicoddemus commented Jan 9, 2018

coveralls commented Jan 9, 2018 • edited Loading

RonnyPfannschmidt commented Jan 17, 2018

Ankitagupta2309 commented Feb 16, 2018

carlos-jenkins commented Feb 20, 2018

carlos-jenkins commented Feb 21, 2018

nicoddemus commented Feb 21, 2018 • edited Loading

carlos-jenkins commented Sep 11, 2017 •

edited

Loading

nicoddemus commented Sep 12, 2017 •

edited

Loading

dajose commented Dec 12, 2017 •

edited

Loading

nicoddemus commented Dec 13, 2017 •

edited

Loading

dajose commented Jan 2, 2018 •

edited

Loading

coveralls commented Jan 9, 2018 •

edited

Loading

nicoddemus commented Feb 21, 2018 •

edited

Loading