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

merge the pytest-cache plugin into core #828

Merged
merged 9 commits into from
Sep 16, 2015

Conversation

RonnyPfannschmidt
Copy link
Member

this is a recreation of the merge that integrates the cache plugin

this time the looponfail mode was left out due to unsolved issues

@nicoddemus
Copy link
Member

This will be an excellent feature for 2.8. 😎

cache: working with cross-testrun state
=======================================

Usage
Copy link
Member

Choose a reason for hiding this comment

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

This session about usage should go into usage.txt, I think

Copy link
Member

Choose a reason for hiding this comment

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

Also, all these new docs should have a .. versionadded:: 2.8 directive to them.

@hpk42
Copy link
Contributor

hpk42 commented Jul 13, 2015

do i miss something or does this PR not yet skip the loading of "pytest-cache" and will thus lead to option confclits?

@RonnyPfannschmidt
Copy link
Member Author

due to naming the plugin cacheprovider we will naturally superseed the external one, which uses the cacheprovider entrypoint, that is not considered since we have a core plugin with the same name

@RonnyPfannschmidt
Copy link
Member Author

pytest-flakes is currently broken due to using json

assert result.ret == 0
result.stdout.fnmatch_lines(["*1 passed*"])

def XXX_test_cachefuncarg(self, testdir):
Copy link
Member

Choose a reason for hiding this comment

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

xfail?

Copy link
Member Author

Choose a reason for hiding this comment

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

that one i probably missed when porting from pytest-cache

Copy link
Member

Choose a reason for hiding this comment

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

I imagined. 😄

(btw, did you notice the comments I made about the docs? Just making sure)

@nicoddemus
Copy link
Member

pytest-flakes is currently broken due to using json

But what's the problem? The flakes environment explicitly uses python2.7, which has json as a builtin module.

@RonnyPfannschmidt
Copy link
Member Author

pytest-flakes pases a set in, thats not json compatible

@nicoddemus
Copy link
Member

pytest-flakes pases a set in, thats not json compatible

Oh so you mean the latest pytest-flakes is broken?

@RonnyPfannschmidt
Copy link
Member Author

pytest-cache originally supported sets and other builtins

@nicoddemus
Copy link
Member

pytest-cache originally supported sets and other builtins

Oh now I undrstand, thanks.

So that's a backward incompatibility change right? We should at least mention this in the docs and in the CHANGELOG.

@@ -1,6 +1,8 @@
2.8.0.dev (compared to 2.7.X)
-----------------------------

- merge the pytest-cache extension into core
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 it's worth adding here how this will affect users with pytest-cache installed.

Also, add a "Thanks Ronny Pfannschmidt for the PR" note here. 😉

@RonnyPfannschmidt RonnyPfannschmidt added type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature plugin: cache related to the cache builtin plugin labels Jul 24, 2015
@RonnyPfannschmidt RonnyPfannschmidt added this to the 2.8 milestone Aug 27, 2015
@RonnyPfannschmidt
Copy link
Member Author

ill finish it up by turning serialization failure into a warning, and providing optional arguments to define the serializer

@@ -124,6 +120,11 @@ def _main(config, session):
config.hook.pytest_collection(session=session)
config.hook.pytest_runtestloop(session=session)

if session.testsfailed:
Copy link
Member

Choose a reason for hiding this comment

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

👍

@nicoddemus
Copy link
Member

ill finish it up by turning serialization failure into a warning, and providing optional arguments to define the serializer

Kudos to the warning, but what you plan to do regarding the serializer options exactly? To allow the user to provide its own serializer class for example?

@RonnyPfannschmidt
Copy link
Member Author

More about providing dumps and loads as arguments

Unfortunately warnings in early usage geht hidden due to a bug, so ill leave the internalererror for now

session.exitstatus = EXIT_TESTSFAILED
elif session.testscollected == 0:
session.exitstatus = EXIT_NOTESTSCOLLECTED

Copy link
Contributor

Choose a reason for hiding this comment

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

is this change of moving this code somewhere else related to pytest cache integrastion?

Copy link
Member Author

Choose a reason for hiding this comment

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

no, i think we should move it to mainline before - @nicoddemus did the original refactoring which i had to partially revert/change to have cache output good return codes for commands unrelated to running tests

@hpk42
Copy link
Contributor

hpk42 commented Sep 16, 2015

To push through with the integration I opened a new PR #1015 which contains doc fixes.

@The-Compiler The-Compiler merged commit e064556 into pytest-dev:master Sep 16, 2015
@RonnyPfannschmidt RonnyPfannschmidt deleted the cache-integration branch September 27, 2015 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin: cache related to the cache builtin plugin type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants