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

Migrate test runner to py.test #569

Merged
merged 2 commits into from Aug 10, 2016

Conversation

Projects
None yet
5 participants
@pferate
Contributor

pferate commented Jul 26, 2016

Maintainer of pytest_gae needs to merge open PR and push to PyPI.
In the mean time, get code directly from GitHub mirror.

@googlebot googlebot added the cla: yes label Jul 26, 2016

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Jul 26, 2016

Member

We can probably just rip off the App Engine stuff from here into a conftest.py for the appengine things.

(it's not a lot of code)

Member

theacodes commented Jul 26, 2016

We can probably just rip off the App Engine stuff from here into a conftest.py for the appengine things.

(it's not a lot of code)

Show outdated Hide outdated oauth2client/crypt.py
@pferate

This comment has been minimized.

Show comment
Hide comment
@pferate

pferate Jul 26, 2016

Contributor

@jonparrott Should we worry about populating the conftest.py file now? Or should we wait until we start moving everything to fixtures (phase 3)?

Contributor

pferate commented Jul 26, 2016

@jonparrott Should we worry about populating the conftest.py file now? Or should we wait until we start moving everything to fixtures (phase 3)?

@pferate

This comment has been minimized.

Show comment
Hide comment
@pferate

pferate Jul 27, 2016

Contributor

There is a bug in the 0.9 release of flake8-import-order (released yesterday) that is causing the flake8 test to incorrectly fail.

I've already created an issue for it.

Contributor

pferate commented Jul 27, 2016

There is a bug in the 0.9 release of flake8-import-order (released yesterday) that is causing the flake8 test to incorrectly fail.

I've already created an issue for it.

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Jul 27, 2016

Member

@pferate if we need app engine stuff to work (which we do) we might as well populate conftest.py just for that. You only need a subset of the stuff in the example I linked.

Member

theacodes commented Jul 27, 2016

@pferate if we need app engine stuff to work (which we do) we might as well populate conftest.py just for that. You only need a subset of the stuff in the example I linked.

@pferate

This comment has been minimized.

Show comment
Hide comment
@pferate

pferate Jul 27, 2016

Contributor

I copied the code from App Engine file you linked. I moved the imports within setup_sdk_imports to within try/except blocks, so that the tests don't fail outside of App Engine.

I just added the fixture code for this PR to pytest_configure().

These will be expanded more when we transition the tests here to fixtures.

Contributor

pferate commented Jul 27, 2016

I copied the code from App Engine file you linked. I moved the imports within setup_sdk_imports to within try/except blocks, so that the tests don't fail outside of App Engine.

I just added the fixture code for this PR to pytest_configure().

These will be expanded more when we transition the tests here to fixtures.

Show outdated Hide outdated tests/conftest.py
Show outdated Hide outdated tests/conftest.py
Show outdated Hide outdated tests/conftest.py
Show outdated Hide outdated tests/conftest.py
Show outdated Hide outdated tests/conftest.py
Show outdated Hide outdated tox.ini

@theacodes theacodes added this to the 4.0.0 milestone Jul 27, 2016

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Jul 27, 2016

Member

Please don't merge this until after the 3.0.0 release. I'll remove the label when it's safe.

Member

theacodes commented Jul 27, 2016

Please don't merge this until after the 3.0.0 release. I'll remove the label when it's safe.

@theacodes theacodes removed the don't merge label Jul 28, 2016

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Jul 28, 2016

Member

v3.0.0 is released so this can now be considered for merging.

Member

theacodes commented Jul 28, 2016

v3.0.0 is released so this can now be considered for merging.

@pferate

This comment has been minimized.

Show comment
Hide comment
@pferate

pferate Jul 28, 2016

Contributor

Should there be a 4.x or Dev branch, so that they can be kept separate from 3.x updates?

Contributor

pferate commented Jul 28, 2016

Should there be a 4.x or Dev branch, so that they can be kept separate from 3.x updates?

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Jul 28, 2016

Member

Should there be a 4.x or Dev branch, so that they can be kept separate from 3.x updates?

I don't think so, but @nathanielmanistaatgoogle is welcome to overrule that.

Member

theacodes commented Jul 28, 2016

Should there be a 4.x or Dev branch, so that they can be kept separate from 3.x updates?

I don't think so, but @nathanielmanistaatgoogle is welcome to overrule that.

@nathanielmanistaatgoogle

This comment has been minimized.

Show comment
Hide comment
@nathanielmanistaatgoogle

nathanielmanistaatgoogle Jul 29, 2016

Contributor

Commit log message should be longer and describe why this change is being made.

Contributor

nathanielmanistaatgoogle commented Jul 29, 2016

Commit log message should be longer and describe why this change is being made.

@nathanielmanistaatgoogle

This comment has been minimized.

Show comment
Hide comment
@nathanielmanistaatgoogle

nathanielmanistaatgoogle Jul 29, 2016

Contributor

No dev branches. No branches other than master and gh_pages, actually. We're not that complicated a project.

Contributor

nathanielmanistaatgoogle commented Jul 29, 2016

No dev branches. No branches other than master and gh_pages, actually. We're not that complicated a project.

@pferate

This comment has been minimized.

Show comment
Hide comment
@pferate

pferate Aug 8, 2016

Contributor

@jonparrott, I've been going over various ways of handling the GAE in this PR and I'd like some feedback.

Initially, I was using a fork of pytest_gae, due to the main library not working with newer versions of app engine. Last week I got a PR merged to fix this and the library owner pushed it to PyPI.

You said to just use the code from the App Engine repo, but not all of it was utilized. I also ran into an issue with the cover tox environment; one test expects GAE to be available and the other expects it to not be available. If we had it always available, there is some trivial code that doesn't execute, because it's expecting the GAE library to not be there. There was no simple way of doing this without reproducing code from a plug-in. Should I add a CLI option to load GAE (which would be added from plug-in)? Also, we don't load the google module directly, so the line about reloading it always gets skipped.

I also came across another plug-in, pytest-beds, which adds the following fixtures:

  • testbed
  • mailer
  • channel
  • urlfetch
  • memcache
  • taskqueue
  • blobstore
  • ndb
  • users

pytest-beds seems like the most flexible option to me.

What are your thoughts on going forward with this?

Contributor

pferate commented Aug 8, 2016

@jonparrott, I've been going over various ways of handling the GAE in this PR and I'd like some feedback.

Initially, I was using a fork of pytest_gae, due to the main library not working with newer versions of app engine. Last week I got a PR merged to fix this and the library owner pushed it to PyPI.

You said to just use the code from the App Engine repo, but not all of it was utilized. I also ran into an issue with the cover tox environment; one test expects GAE to be available and the other expects it to not be available. If we had it always available, there is some trivial code that doesn't execute, because it's expecting the GAE library to not be there. There was no simple way of doing this without reproducing code from a plug-in. Should I add a CLI option to load GAE (which would be added from plug-in)? Also, we don't load the google module directly, so the line about reloading it always gets skipped.

I also came across another plug-in, pytest-beds, which adds the following fixtures:

  • testbed
  • mailer
  • channel
  • urlfetch
  • memcache
  • taskqueue
  • blobstore
  • ndb
  • users

pytest-beds seems like the most flexible option to me.

What are your thoughts on going forward with this?

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Aug 8, 2016

Member

@pferate I'm going to send a PR to your fork to get the App Engine stuff where I want it.

Member

theacodes commented Aug 8, 2016

@pferate I'm going to send a PR to your fork to get the App Engine stuff where I want it.

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes
Member

theacodes commented Aug 8, 2016

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Aug 8, 2016

Member

I am now okay with this. @nathanielmanistaatgoogle to do the final review and merge.

(also there's a conflict, please fix)

Member

theacodes commented Aug 8, 2016

I am now okay with this. @nathanielmanistaatgoogle to do the final review and merge.

(also there's a conflict, please fix)

@pferate

This comment has been minimized.

Show comment
Hide comment
@pferate

pferate Aug 8, 2016

Contributor

Rebased onto master.

Thanks for the help @jonparrott!

Contributor

pferate commented Aug 8, 2016

Rebased onto master.

Thanks for the help @jonparrott!

@nathanielmanistaatgoogle

This comment has been minimized.

Show comment
Hide comment
@nathanielmanistaatgoogle

nathanielmanistaatgoogle Aug 9, 2016

Contributor

Looks mostly good; just that one comment.

Contributor

nathanielmanistaatgoogle commented Aug 9, 2016

Looks mostly good; just that one comment.

@pferate

This comment has been minimized.

Show comment
Hide comment
@pferate

pferate Aug 9, 2016

Contributor

@jonparrott, with us moving the app engine tests into their own directory, should we move the app engine modules into their own directory as well?

Another coupled set of files are contrib/gce and contrib/_metadata.

Contributor

pferate commented Aug 9, 2016

@jonparrott, with us moving the app engine tests into their own directory, should we move the app engine modules into their own directory as well?

Another coupled set of files are contrib/gce and contrib/_metadata.

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Aug 9, 2016

Member

with us moving the app engine tests into their own directory, should we move the app engine modules into their own directory as well?

I don't think so. There's no hard and fast rule that tests must be organized exactly as their module-under-test. I think defaulting to that but making exceptions where it makes testing easier (like we did here) makes sense. Practicality beats purity.

Member

theacodes commented Aug 9, 2016

with us moving the app engine tests into their own directory, should we move the app engine modules into their own directory as well?

I don't think so. There's no hard and fast rule that tests must be organized exactly as their module-under-test. I think defaulting to that but making exceptions where it makes testing easier (like we did here) makes sense. Practicality beats purity.

set_up_gae_environment(config.getoption('gae_sdk'))
def pytest_ignore_collect(path, config):

This comment has been minimized.

@nathanielmanistaatgoogle

nathanielmanistaatgoogle Aug 9, 2016

Contributor

Am I right to learn from there being no call sites of this function in added code that this, due to its name, is called by py.test? If so, maybe a better doc string for it would be something like Directs py.test to skip App Engine tests when --gae-sdk is not specified.?

@nathanielmanistaatgoogle

nathanielmanistaatgoogle Aug 9, 2016

Contributor

Am I right to learn from there being no call sites of this function in added code that this, due to its name, is called by py.test? If so, maybe a better doc string for it would be something like Directs py.test to skip App Engine tests when --gae-sdk is not specified.?

@pferate

This comment has been minimized.

Show comment
Hide comment
@pferate

pferate Aug 9, 2016

Contributor

@nathanielmanistaatgoogle: PR updated, PTAL.

Contributor

pferate commented Aug 9, 2016

@nathanielmanistaatgoogle: PR updated, PTAL.

@nathanielmanistaatgoogle

This comment has been minimized.

Show comment
Hide comment
@nathanielmanistaatgoogle

nathanielmanistaatgoogle Aug 9, 2016

Contributor

One last tweak; sorry.

Contributor

nathanielmanistaatgoogle commented Aug 9, 2016

One last tweak; sorry.

@pferate

This comment has been minimized.

Show comment
Hide comment
@pferate

pferate Aug 9, 2016

Contributor

PR updated. @nathanielmanistaatgoogle PTAL.

Contributor

pferate commented Aug 9, 2016

PR updated. @nathanielmanistaatgoogle PTAL.

@nathanielmanistaatgoogle

This comment has been minimized.

Show comment
Hide comment
@nathanielmanistaatgoogle

nathanielmanistaatgoogle Aug 9, 2016

Contributor

What's with that test failure?

Is it desired that this go in as two commits? (No pushing in either direction; just want to confirm.)

Contributor

nathanielmanistaatgoogle commented Aug 9, 2016

What's with that test failure?

Is it desired that this go in as two commits? (No pushing in either direction; just want to confirm.)

@pferate

This comment has been minimized.

Show comment
Hide comment
@pferate

pferate Aug 9, 2016

Contributor

Looks like it may be related to #590. This is what is in my Travis logs:

# Travis ships with an old version of PyPy, so install at least version 2.6.
if [[ "${TOX_ENV}" == "pypy" ]]; then
    git clone https://github.com/yyuu/pyenv.git ${HOME}/.pyenv
    ${HOME}/.pyenv/bin/pyenv install pypy-2.6.0
fi
fatal: destination path '/home/travis/.pyenv' already exists and is not an empty directory.

In my repo, pypy passed the first time, but failed when I reran the test.

As for the commits, one was a merged PR from @jonparrott helping me sort out some of the GAE tests, so I didn't want to lose his work within mine. Functionally, they could be squashed.

Contributor

pferate commented Aug 9, 2016

Looks like it may be related to #590. This is what is in my Travis logs:

# Travis ships with an old version of PyPy, so install at least version 2.6.
if [[ "${TOX_ENV}" == "pypy" ]]; then
    git clone https://github.com/yyuu/pyenv.git ${HOME}/.pyenv
    ${HOME}/.pyenv/bin/pyenv install pypy-2.6.0
fi
fatal: destination path '/home/travis/.pyenv' already exists and is not an empty directory.

In my repo, pypy passed the first time, but failed when I reran the test.

As for the commits, one was a merged PR from @jonparrott helping me sort out some of the GAE tests, so I didn't want to lose his work within mine. Functionally, they could be squashed.

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Aug 9, 2016

Member

The pypy failure is my fault. We probably just need to check for the non-existence of {$HOME}/.pyenv before doing the git clone.

We can do it in this PR or I can send a separate one.

Member

theacodes commented Aug 9, 2016

The pypy failure is my fault. We probably just need to check for the non-existence of {$HOME}/.pyenv before doing the git clone.

We can do it in this PR or I can send a separate one.

@nathanielmanistaatgoogle

This comment has been minimized.

Show comment
Hide comment
@nathanielmanistaatgoogle

nathanielmanistaatgoogle Aug 9, 2016

Contributor

Separate pull request with the fix in isolation, please.

Contributor

nathanielmanistaatgoogle commented Aug 9, 2016

Separate pull request with the fix in isolation, please.

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Aug 9, 2016

Member

@pferate It's fixed, rebase and try again?

Member

theacodes commented Aug 9, 2016

@pferate It's fixed, rebase and try again?

pferate and others added some commits Jul 26, 2016

Migrate test runner to py.test
Migrating test runner from `unittest2`/`nose` to `pytest`.
The pytest runner is also compatible with both unittest and nose tests.
Some of the benefits of PyTest include:
    * using plain asserts
    * function-based fixtures instead of setUp and tearDown
    * no strange camelCase methods
@pferate

This comment has been minimized.

Show comment
Hide comment
@pferate

pferate Aug 9, 2016

Contributor

@jonparrott Rebased and waiting on Travis.

One weird thing that I've been seeing recently is py33 tests have been failing in my fork on Travis, but have been passing on the main repo. They also pass when I run them on my local system.

The 2 linked Travis jobs are from the same commit from yesterday. It looks like my fork is collecting the django tests (it shouldn't) and the main repo correctly skips them. Before I dig much further, do you have any idea of what's going on?

As long as it passes on the main repo, it doesn't really matter; but this seems strange.

Contributor

pferate commented Aug 9, 2016

@jonparrott Rebased and waiting on Travis.

One weird thing that I've been seeing recently is py33 tests have been failing in my fork on Travis, but have been passing on the main repo. They also pass when I run them on my local system.

The 2 linked Travis jobs are from the same commit from yesterday. It looks like my fork is collecting the django tests (it shouldn't) and the main repo correctly skips them. Before I dig much further, do you have any idea of what's going on?

As long as it passes on the main repo, it doesn't really matter; but this seems strange.

@nathanielmanistaatgoogle

This comment has been minimized.

Show comment
Hide comment
@nathanielmanistaatgoogle

nathanielmanistaatgoogle Aug 9, 2016

Contributor

@pferate: are you waiting for answers from @jonparrott before this gets merged?

(I am completely fine with the pull request content and metadata; feel free to merge when conversation is again concluded.)

Contributor

nathanielmanistaatgoogle commented Aug 9, 2016

@pferate: are you waiting for answers from @jonparrott before this gets merged?

(I am completely fine with the pull request content and metadata; feel free to merge when conversation is again concluded.)

@pferate

This comment has been minimized.

Show comment
Hide comment
@pferate

pferate Aug 9, 2016

Contributor

No need to wait. I just had some weird, unique test results that I'm trying to figure out on my end.

Contributor

pferate commented Aug 9, 2016

No need to wait. I just had some weird, unique test results that I'm trying to figure out on my end.

@nathanielmanistaatgoogle nathanielmanistaatgoogle merged commit f774a66 into googleapis:master Aug 10, 2016

3 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details

@pferate pferate deleted the pferate:pytest_runner branch Aug 10, 2016

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