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

Xfail marker for non-standard Item missing `obj` #2231

Closed
vidartf opened this Issue Feb 3, 2017 · 3 comments

Comments

Projects
None yet
2 participants
@vidartf
Contributor

vidartf commented Feb 3, 2017

Hi!

We're currently writing a plugin for py.test that collects custom tests (i.e. our own class that inherits from pytest.Item). I want to add an xfail marker to these, so I try to do this dynamically via item.add_marker(). If I do this, and run the tests, it will fail on this line:

d.update(self.item.obj.__globals__)

Here, the skipping plugin assumes that the item has an obj attribute, which again has a __globals__ attribute. I can side-step this issue by simply adding a dummy variable here, with an empty __globals__ dict, but I want to ensure that:

  • I am not doing something stupid by adding the dummy
  • I am not doing something wrong elsewhere since I am missing the obj attribute
  • I report this so that I minimize the risk of our plugin failing for similar reasons in the future

Am I doing it right? 😄

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Feb 3, 2017

I think the quick workaround is adding a dummy object like you propose.

But I also think skipping shouldn't require that obj attribute at all since it is not part of Item's interface.

Would you mind to submit a PR which removes that requirement from there? I think this should do it:

def _getglobals(self):
    d = {'os': os, 'sys': sys, 'config': self.item.config}
    if hasattr(self.item, 'obj'):
        d.update(self.item.obj.__globals__)
    return d

What do you think @RonnyPfannschmidt?

vidartf added a commit to vidartf/pytest that referenced this issue Feb 3, 2017

@vidartf

This comment has been minimized.

Contributor

vidartf commented Feb 3, 2017

I see now that an alternative solution was proposed in #2142 .

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Feb 3, 2017

I think they complement each other, with your's fixing the underlying problem more correctly. 👍

This was referenced Mar 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment