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.

Sign up for GitHub

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

Jump to bottom

Split into modules #117

Merged
merged 7 commits into from
Jan 8, 2018
Merged

Split into modules #117

merged 7 commits into from
Jan 8, 2018

Conversation

goodboy
Copy link
Contributor

@goodboy goodboy commented Jan 7, 2018

This is to replace #101 by @RonnyPfannschmidt.

The differences include:

  • rebased onto master incorporating the latest changes
  • maintaining as much of the history as possible in what was pluggy.PluginManager (where most of the non-@hpk42 contributions seemed to be) by moving pluggy.__init__.py -> pluggy.manager and pulling out internals from that module into other new modules
  • ensuring the test suite can run successfully between each incremental change (commit) such that each patch is a logical change set
  • avoiding the first iteration - split more internal utilities out of the main package file #101 proposed parse_funcs module (it's unecessary when it's a utility function used only by the code in hooks.py)

Let me know what you guys think!

@nicoddemus
Copy link
Member

The split seems sensible to me... we just have to take a look at the test failures with pytest master.

@goodboy
Copy link
Contributor Author

goodboy commented Jan 7, 2018 •
edited
Loading

we just have to take a look at the test failures with pytest master

Yeah gonna try and reproduce locally with tox.
It's also weird that my latest force push hasn't shown up here in GH...

@goodboy
Copy link
Contributor Author

goodboy commented Jan 7, 2018

@nicoddemus hah! Think I figured it out.
The first time you run the pytest integration tests they fail because on install I think the old pluggy package is being used somehow. The failure is consistent as long as I pass -r to tox.

My original guess would be that the _PYTEST_SETUP_SKIP_PLUGGY_DEP isn't being set early enough since when I hop in the debugger in fact pytest is using the pluggy installed in the virtualenv instead of the sources. However, inspecting that env var shows it is set by the time it hits the import error?

Any ideas?

@goodboy
Copy link
Contributor Author

goodboy commented Jan 7, 2018 •
edited
Loading

@obestwalter Did something change in tox regarding when/how the project under test is installed?
Maybe related to tox-dev/tox#571?

We're doing something weird with pluggy where we install a dependent project (pytest) to make sure pluggy doesn't break it but pytest also has pluggy as a dep. Seems to me something is different with how the installation of the local pluggy under test is being installed?

@nicoddemus
Copy link
Member

@tgoodlet a test of your theory would be to set _PYTEST_SETUP_SKIP_PLUGGY_DEP in the .travis.yml file manually before calling tox; if that changes the outcome then we know your hypothesis holds.

@goodboy
Copy link
Contributor Author

goodboy commented Jan 7, 2018 •
edited
Loading

My theory failed...

Also old tox versions don't seem to make a difference.
I'm going to try and hook into my local pytest code and see what's going on.

@goodboy goodboy mentioned this pull request Jan 7, 2018
Merged
@goodboy
Copy link
Contributor Author

goodboy commented Jan 7, 2018

Oh man @nicoddemus 🤦‍♂️
You're gonna laugh at me...

RonnyPfannschmidt and others added 6 commits January 7, 2018 16:38
@RonnyPfannschmidt
move tracer to own file
d2e19a4
Move package file to pluggy.manager
8984e86 
This begins the splitting of internals into more sensibly named modules.
This first move is to preserve as much history in the original large
package module as possible.
Re-package such that tests work
3f1eda5
Move hook internals to a seperate module
dfedf4a
Move hook execution tracer to tracing module
bfc2ae3
Expose proper public imports and fix tests to match
0a959f8
@nicoddemus
Copy link
Member

What is it? 😛

@goodboy
Copy link
Contributor Author

goodboy commented Jan 7, 2018

minor version bump was needed to get pip to install the sources...

@nicoddemus
Copy link
Member

Oh hehehe makes sense! Glad it was something simple then! 😁

@goodboy
Copy link
Contributor Author

goodboy commented Jan 7, 2018

Alright now I just need some approvals 👍

@goodboy goodboy self-assigned this Jan 7, 2018
@nicoddemus
Copy link
Member

Nice work, thanks @tgoodlet!

@goodboy
Copy link
Contributor Author

goodboy commented Jan 8, 2018

@RonnyPfannschmidt @hpk42 you two ok with this?

@goodboy goodboy merged commit 32ee0e0 into pytest-dev:master Jan 8, 2018
@goodboy goodboy mentioned this pull request Jan 8, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicoddemus nicoddemus nicoddemus approved these changes

@RonnyPfannschmidt RonnyPfannschmidt RonnyPfannschmidt approved these changes

@hpk42 hpk42 Awaiting requested review from hpk42

Assignees

@goodboy goodboy

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@goodboy @nicoddemus @RonnyPfannschmidt