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

Add test environment using pluggy from master branch #2744

Merged
merged 7 commits into from
Sep 6, 2017

Conversation

nicoddemus
Copy link
Member

Fix #2737

After merging this we can conclude pytest-dev/pluggy#79.

For some reason, the previous approach brakes 'coveralls' because
pip still tries to install the 'pluggy' master requirement
(git+https://...)
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.005%) to 92.117% when pulling 9bbf14d on nicoddemus:pluggy-master into 9d373d8 on pytest-dev:features.

Copy link

@goodboy goodboy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you've got one API mismatch and maybe a slight factoring you can do in the tox.ini other wise looks good.

rep.headerlines += ["header1"]
return rep
outcome.set_result(rep)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nicoddemus that method doesn't exist I don't think?
It's force_result().

I do, however, prefer this name better and think we should move to it.
It fits with Future.set_result() which may be something that's handy if we ever want to offer an async API.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think the current name is much better explaining whats going on as a futures set_result will raise a exception on second use

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RonnyPfannschmidt fair enough.

Either way I'm confused how this test is passing if the method name is wrong?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I will have to take a closer look later, indeed this is weird.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh of course, it raises an error at the rep.headerlines += ["header1"] line. Plus it is an "xfail" test so it is not really running.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm updating to force_result anyway to avoid future confusion.

tox.ini Outdated
[testenv:py35-pluggymaster]
passenv={[testenv]passenv}
commands={[testenv]commands}
setenv=
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can these two be pulled from the py27 job above?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, will do.

Even though the test is not running at the moment (xfail), at least
we avoid future confusion
@nicoddemus
Copy link
Member Author

Done requested changes.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 92.111% when pulling 7d59b2e on nicoddemus:pluggy-master into 9d373d8 on pytest-dev:features.

@RonnyPfannschmidt
Copy link
Member

@nicoddemus please rebase

commands={[testenv:py27-pluggymaster]commands}
setenv=
_PYTEST_SETUP_SKIP_PLUGGY_DEP=1
deps =
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nicoddemus heh I was actually thinking to derive setenv and deps just because they're common to these two envs.

Not a big deal though :)

Copy link

@goodboy goodboy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as that call to force_result() actually made a difference I think this is good 👍

@nicoddemus
Copy link
Member Author

Fixed the merge conflict. 😓

@nicoddemus
Copy link
Member Author

After this is merged, we should re-trigger pluggy's master to ensure it is fixed.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.005%) to 92.049% when pulling d01f08e on nicoddemus:pluggy-master into e1f2254 on pytest-dev:features.

@nicoddemus nicoddemus merged commit b55a4f8 into pytest-dev:features Sep 6, 2017
@nicoddemus nicoddemus deleted the pluggy-master branch September 6, 2017 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants