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

Clean up state after in process pytest runs #3016

Merged
merged 3 commits into from Dec 18, 2017

Conversation

Projects
None yet
4 participants
@jurko-gospodnetic
Contributor

jurko-gospodnetic commented Dec 9, 2017

Internal pytest test suite improvements:

  • cleaned up Testdir taking snapshots & restoring global Python state - extracted to new CwdSnapshot, SysModulesSnapshot & SysPathsSnapshot classes, each saving the state they are interested in on instantiation and restoring it in its restore() method
  • fixed restoring Python state after in-process pytest runs
    • now each in-process pytest run saves a snapshot of important global Python state and restores it after the test completes, including the list of loaded modules & the Python path settings
    • previously only the loaded package data was getting restored, but that was also reverting any loaded package changes done in the test triggering the pytest runs, and not only those done by the pytest runs themselves
    • updated acceptance tests broken by this change, which were only passing before by accident as they were making multiple pytest runs with later ones depending on sys.path changes left behind by the initial one

Changes:

  • _pytest/pytested.py - main change
  • testing/acceptance_test.py - updated + slightly improved broken tests
  • testing/deprecated_test.py - updated broken test
  • testing/test_pytester.py - improved existing tests including added tests for the new functionality
@coveralls

This comment has been minimized.

coveralls commented Dec 9, 2017

Coverage Status

Coverage increased (+0.02%) to 92.718% when pulling 7eb9760 on jurko-gospodnetic:clean-up-state-after-in-process-pytest-runs into 964c29c on pytest-dev:master.

@coveralls

This comment has been minimized.

coveralls commented Dec 9, 2017

Coverage Status

Coverage increased (+0.02%) to 92.718% when pulling 0e6ce01 on jurko-gospodnetic:clean-up-state-after-in-process-pytest-runs into 964c29c on pytest-dev:master.

@coveralls

This comment has been minimized.

coveralls commented Dec 11, 2017

Coverage Status

Coverage increased (+0.02%) to 92.718% when pulling 79c80a0 on jurko-gospodnetic:clean-up-state-after-in-process-pytest-runs into 964c29c on pytest-dev:master.

@jurko-gospodnetic jurko-gospodnetic changed the title from Clean up state after in process pytest runs (to be merged after #3015) to Clean up state after in process pytest runs Dec 12, 2017

@nicoddemus

Thanks @jurko-gospodnetic, this looks like a nice cleanup!

I think this should go into features though as it might break test suites which use pytester, like it did happen with pytest's own test suite.

I don't have time to make a more in depth review right now (I should be in bed already), hopefully I will have more time tomorrow.

Thanks again!

@@ -0,0 +1,7 @@
internal pytest test suite improvements:

This comment has been minimized.

@nicoddemus

nicoddemus Dec 12, 2017

Member

I don't think we can have sub-items in newsfiles like this because it will break up the RST lint checker. You can run inv changelog to update the changelog and check it yourself.

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic Dec 12, 2017

Contributor

Took a while to figure out what inv changelog meant or how to get it working for the project. You really should add a tox environment for this and add a note in the contributing docs on how to run all these processes.

needed to:

  • figure out where the change log is generated from
  • what invoke was
  • what I need to have installed to run it
  • what arguments to pass to invoke

in the end, the best way seems to have been:

  • create an environment
  • install the packages from tasks/requirements.txt
  • run towncrier --draft to see the generated changelog in the console output without anything else getting modified in the project
@coveralls

This comment has been minimized.

coveralls commented Dec 12, 2017

Coverage Status

Coverage increased (+0.02%) to 92.766% when pulling 3e98082 on jurko-gospodnetic:clean-up-state-after-in-process-pytest-runs into 5c6d773 on pytest-dev:master.

@jurko-gospodnetic

This comment has been minimized.

Contributor

jurko-gospodnetic commented Dec 12, 2017

@nicoddemus - as for targeting the features branch with this - tests this would break are only those making multiple in-process pytest runs with later ones depending on earlier ones leaking global interpreter changes, or those expecting pytest runs to remove modules imported in the external test code from sys.modules - both of which seem like buggy/accidental behaviours

But I'll move it to features and we can move it back if you agree...

@jurko-gospodnetic jurko-gospodnetic changed the base branch from master to features Dec 12, 2017

@jurko-gospodnetic

This comment has been minimized.

Contributor

jurko-gospodnetic commented Dec 12, 2017

@nicoddemus - now rebased on top of the features branch and reworded/reduced the changelog entry

@coveralls

This comment has been minimized.

coveralls commented Dec 12, 2017

Coverage Status

Coverage increased (+0.02%) to 92.766% when pulling 35aa325 on jurko-gospodnetic:clean-up-state-after-in-process-pytest-runs into f8f1a52 on pytest-dev:features.

@coveralls

This comment has been minimized.

coveralls commented Dec 12, 2017

Coverage Status

Coverage increased (+0.02%) to 92.766% when pulling 35aa325 on jurko-gospodnetic:clean-up-state-after-in-process-pytest-runs into f8f1a52 on pytest-dev:features.

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Dec 13, 2017

@jurko-gospodnetic thanks for retargeting the branch.

I understand your point of view, but we do our best to avoid breaking existing test suites even if they are working by accident. 👍

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Dec 13, 2017

I will review this as soon as I have some time, thanks for all the hard work you put into this, we appreciate it!

@nicoddemus

LGTM, thanks @jurko-gospodnetic!

# Any sys.module or sys.path changes done while running py.test
# inline should be reverted after the test run completes to avoid
# clashing with later inline tests run within the same pytest test,

This comment has been minimized.

@nicoddemus
@coveralls

This comment has been minimized.

coveralls commented Dec 14, 2017

Coverage Status

Coverage increased (+0.02%) to 92.766% when pulling fc6dc4d on jurko-gospodnetic:clean-up-state-after-in-process-pytest-runs into f8f1a52 on pytest-dev:features.

@jurko-gospodnetic

This comment has been minimized.

Contributor

jurko-gospodnetic commented Dec 17, 2017

  • squashed the changelog fix commit
  • changed one of my @classmethod decorators in test_pytester.py to @staticmethod
  • rebased on top of the current features branch

jurko-gospodnetic added some commits Dec 1, 2017

clean up Testdir taking snapshots & restoring global Python state
Now extracted to new CwdSnapshot, SysModulesSnapshot & SysPathsSnapshot
classes, each saving the state they are interested in on instantiation
and restoring it in its `restore()` method.
fix restoring Python state after in-process pytest runs
Now each in-process pytest run saves a snapshot of important global Python
state and restores it after the test completes, including the list of loaded
modules & the Python path settings.

Previously only the loaded package data was getting restored, but that was
also reverting any loaded package changes done in the test triggering the
pytest runs, and not only those done by the pytest runs themselves.

Updated acceptance tests broken by this change, which were only passing before
by accident as they were making multiple pytest runs with later ones depending
on sys.path changes left behind by the initial one.
@coveralls

This comment has been minimized.

coveralls commented Dec 17, 2017

Coverage Status

Coverage increased (+0.02%) to 92.613% when pulling d85a3ca on jurko-gospodnetic:clean-up-state-after-in-process-pytest-runs into d872791 on pytest-dev:features.

@coveralls

This comment has been minimized.

coveralls commented Dec 17, 2017

Coverage Status

Coverage increased (+0.02%) to 92.613% when pulling d85a3ca on jurko-gospodnetic:clean-up-state-after-in-process-pytest-runs into d872791 on pytest-dev:features.

@RonnyPfannschmidt RonnyPfannschmidt merged commit 0d83dd1 into pytest-dev:features Dec 18, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@RonnyPfannschmidt

This comment has been minimized.

Member

RonnyPfannschmidt commented Dec 18, 2017

well done ! thanks

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