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

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

Merged
merged 3 commits into from Feb 22, 2018

Conversation

Projects
None yet
6 participants
@carlos-jenkins
Contributor

carlos-jenkins commented Sep 11, 2017

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

This comment has been minimized.

Contributor

carlos-jenkins commented Sep 11, 2017

@RonnyPfannschmidt totally reasonable. I'll add those.

Thanks for the review.

@carlos-jenkins

This comment has been minimized.

Contributor

carlos-jenkins commented Sep 11, 2017

@RonnyPfannschmidt deprecation warnings added. Back to you.

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Sep 12, 2017

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:

  1. Agree we should deprecate record_xml_property, thanks for tackling it.
  2. 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.
  3. 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

This comment has been minimized.

Member

nicoddemus commented Nov 10, 2017

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

@carlos-jenkins

This comment has been minimized.

Contributor

carlos-jenkins commented Nov 13, 2017

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:

  1. Great.
  2. 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.
  3. 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

This comment has been minimized.

Member

nicoddemus commented Nov 13, 2017

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

This comment has been minimized.

dajose commented Dec 12, 2017

hey, I just solved some merge conflicts :)

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

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Dec 13, 2017

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

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Dec 13, 2017

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

@RonnyPfannschmidt

This comment has been minimized.

Member

RonnyPfannschmidt commented Dec 13, 2017

it seems doc-tests lack the feature right now

@@ -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 = []

This comment has been minimized.

@RonnyPfannschmidt

RonnyPfannschmidt Dec 13, 2017

Member

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)

This comment has been minimized.

@dajose

dajose Jan 2, 2018

Thanks for the idea :)

I just added a commit with that change

original problem was nicely handled

@coveralls

This comment has been minimized.

coveralls commented Jan 2, 2018

Coverage Status

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

@dajose

This comment has been minimized.

dajose commented Jan 2, 2018

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

This comment has been minimized.

Member

nicoddemus commented Jan 9, 2018

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

This comment has been minimized.

Member

nicoddemus commented Jan 9, 2018

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

@dajose

This comment has been minimized.

dajose commented Jan 9, 2018

@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. 👍

@@ -315,7 +316,8 @@ class TestReport(BaseReport):
"""
def __init__(self, nodeid, location, keywords, outcome,
longrepr, when, sections=(), duration=0, **extra):
longrepr, when, user_properties,

This comment has been minimized.

@nicoddemus

nicoddemus Jan 9, 2018

Member

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

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Jan 9, 2018

@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

This comment has been minimized.

coveralls commented Jan 9, 2018

Coverage Status

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

@carlos-jenkins carlos-jenkins changed the base branch from master to features Jan 9, 2018

@RonnyPfannschmidt

This comment has been minimized.

Member

RonnyPfannschmidt commented Jan 17, 2018

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

@Ankitagupta2309

This comment has been minimized.

Ankitagupta2309 commented Feb 16, 2018

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.

Renamed the fixture record_xml_property to record_property and adapte…
…d logic so that the properties are passed to the TestReport object and thus allow compatibility with pytest-xdist.
@carlos-jenkins

This comment has been minimized.

Contributor

carlos-jenkins commented Feb 20, 2018

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

Add note about deprecating record_xml_property
Also make record_xml_property return record_property directly
@carlos-jenkins

This comment has been minimized.

Contributor

carlos-jenkins commented Feb 21, 2018

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Feb 21, 2018

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! 👍

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment