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

Clean up state after in process pytest runs #3016

Conversation

jurko-gospodnetic
Copy link
Contributor

@jurko-gospodnetic 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
Copy link

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
Copy link

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.

@jurko-gospodnetic jurko-gospodnetic force-pushed the clean-up-state-after-in-process-pytest-runs branch from 0e6ce01 to 79c80a0 Compare December 11, 2017 14:11
@coveralls
Copy link

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 force-pushed the clean-up-state-after-in-process-pytest-runs branch from 79c80a0 to 3e98082 Compare December 12, 2017 00:19
@jurko-gospodnetic jurko-gospodnetic changed the title Clean up state after in process pytest runs (to be merged after #3015) Clean up state after in process pytest runs Dec 12, 2017
Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

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:
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link

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
Copy link
Contributor Author

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 force-pushed the clean-up-state-after-in-process-pytest-runs branch 2 times, most recently from 5acf160 to 35aa325 Compare December 12, 2017 12:30
@jurko-gospodnetic jurko-gospodnetic changed the base branch from master to features December 12, 2017 12:31
@jurko-gospodnetic
Copy link
Contributor Author

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

@coveralls
Copy link

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
Copy link

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
Copy link
Member

@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
Copy link
Member

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

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

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,
Copy link
Member

Choose a reason for hiding this comment

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

👍

@coveralls
Copy link

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 jurko-gospodnetic force-pushed the clean-up-state-after-in-process-pytest-runs branch from fc6dc4d to d7f3007 Compare December 17, 2017 11:45
@jurko-gospodnetic
Copy link
Contributor Author

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

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.
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.
@jurko-gospodnetic jurko-gospodnetic force-pushed the clean-up-state-after-in-process-pytest-runs branch from d7f3007 to d85a3ca Compare December 17, 2017 11:48
@coveralls
Copy link

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
Copy link

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
Copy link
Member

well done ! thanks

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