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

Run tests in clean virtual environment [version without tox] #164

Merged
merged 13 commits into from Oct 13, 2019

Conversation

astrofrog
Copy link
Contributor

@joerick - This is an alternative to #162 that does not use tox - however I am not able to get all the CI services to pass currently.

@YannickJadoul
Copy link
Member

I've been running some more debugging test on this PR, and the problem seems to be something similar to pypa/virtualenv#620: the wrong shebang line somehow gets put into the nosetests script; https://travis-ci.org/YannickJadoul/cibuildwheel/jobs/583754350#L3308

Not sure why or how, though :-|

@YannickJadoul
Copy link
Member

@astrofrog Found a solution! https://travis-ci.org/YannickJadoul/cibuildwheel/builds/583880457 is succeeding with minimal changes!

I just don't know why or how, but I also feel like I don't really want to know. Something ugly seems to be going on with this __PYVENV_LAUNCHER__ environment variable (but only on Python 3, on macOS, sómehow...).

Here's the commit on my version of your branch. Feel free to take the commit (or just the changes) and add it here :-) I'd be up for using this PR instead of the one with tox (#162).

@YannickJadoul
Copy link
Member

A few more things I noticed:

  • Maybe copying the env directory before changing the PATH would be good to keep the build env separate from the test env? I don't know if in some situation, we'd later need the original, unmodified env again.
  • The whole setting up of a virtualenv could be inside an if statement on whether we are actually testing something. If we don't have a CIBW_TEST_COMMAND, there's no point creating a virtualenv, is there?
  • It does take longer now to run all tests, it seems. Not sure if that's a big problem, but it's worth noticing.

@joerick
Copy link
Contributor

joerick commented Sep 14, 2019

Agree with @YannickJadoul comments - I also like this version better.

We should keep the venv environment variables isolated from the rest of the program - that might be a test_env copy dict in the macos/windows case, or a subshell in the bash case to run source env/bin/activate in.

@joerick
Copy link
Contributor

joerick commented Oct 12, 2019

I hope it's okay if I jump in here - the PRs are piling up so I'd like to get things merged if I can! Going to merge @YannickJadoul's commit and fix things up as per his comment.

@YannickJadoul
Copy link
Member

Looks good!

One more thing: now we're not installing the wheel if it's not used for the tests, and I agree that building the virtual environment takes to long to just install the wheel without anything else.
Installing the wheel seemed like some kind of most basic test whether the wheel was the right version and not corrupt or anything. Is there a way to add a check like that again? But maybe/probably that install would never fail, and I'm overestimating what that install would do?

@joerick
Copy link
Contributor

joerick commented Oct 12, 2019

Is there a way to add a check like that again? But maybe/probably that install would never fail, and I'm overestimating what that install would do?

I think a wheel install is just checking the tags and unzipping the archive, so I'm not sure that's catching very much... unless anyone can report having caught problems at the pip install <wheel> step?

@YannickJadoul
Copy link
Member

I think a wheel install is just checking the tags and unzipping the archive, so I'm not sure that's catching very much...

Yeah, probably. So if there was no reason you had it installed outside of the if test_command: before, we're good to go.

@joerick joerick merged commit 5f416b1 into pypa:master Oct 13, 2019
@joerick
Copy link
Contributor

joerick commented Oct 13, 2019

🎉 merged! thanks @astrofrog & @YannickJadoul !

nsoranzo added a commit to nsoranzo/pysam that referenced this pull request Nov 8, 2019
So the test configured in `CIBW_TEST_COMMAND` runs in a clean
environment: pypa/cibuildwheel#164
nsoranzo added a commit to nsoranzo/pysam that referenced this pull request Nov 8, 2019
So the test configured in `CIBW_TEST_COMMAND` runs in a clean
environment: pypa/cibuildwheel#164
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.

None yet

3 participants