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

Excluding system tests from being installed. #1373

Merged
merged 1 commit into from
Feb 12, 2016

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Jan 8, 2016

@tseaver I noticed this while trying to from system_tests import populate_datastore and the import failed due to reasons that were "resolved" in #1349.

Then I peaked and realized that the error was coming from

/usr/local/lib/python2.7/dist-packages/system_tests/populate_datastore.py
/usr/local/lib/python2.7/dist-packages/gcloud/datastore/client.py

Notice that in our repo:

>>> from setuptools import find_packages
>>> find_packages()
['gcloud',
 'system_tests',
 'gcloud.storage',
 'gcloud.pubsub',
 'gcloud.dns',
 'gcloud.bigtable',
 'gcloud.bigquery',
 'gcloud.resource_manager',
 'gcloud.search',
 'gcloud.datastore',
 'gcloud.streaming',
 'gcloud.storage.demo',
 'gcloud.bigtable.happybase',
 'gcloud.bigtable._generated',
 'gcloud.datastore.demo',
 'gcloud.datastore._generated']

(This is due to the fact that system_tests/__init__.py was added in #954 / #273)

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 8, 2016
@tseaver
Copy link
Contributor

tseaver commented Jan 8, 2016

Note that excluding in MANIFEST.in will exclude them from the sdist, which can make it hard for downstreams to test (if that matters).

On what host were those /usr/local/lib/python2.7/dist-lib/ paths found?

@dhermes
Copy link
Contributor Author

dhermes commented Jan 8, 2016

Ubuntu 14.04

@dhermes dhermes force-pushed the exclude-sys-tests branch from fcca14a to 65c0ef8 Compare January 9, 2016 01:45
@dhermes
Copy link
Contributor Author

dhermes commented Jan 9, 2016

@tseaver PTAL. I did a hacky thing my modifying sys.path in __main__.

@tseaver
Copy link
Contributor

tseaver commented Jan 14, 2016

We still don't want to ship /usr/local/lib/python2.7/dist-packages/system_tests/ to people

You mean we don't want those files to be there after they run sudo pip install gcloud? I'm thinking of downstream packagers, who might want to be able to run all the tests after creating a .deb/.rpm from the sdist.

@dhermes
Copy link
Contributor Author

dhermes commented Jan 14, 2016

You mean we don't want those files to be there after they run sudo pip install gcloud

Yep. Is there a middle ground?

@dhermes
Copy link
Contributor Author

dhermes commented Jan 15, 2016

@tseaver Bump

1 similar comment
@dhermes
Copy link
Contributor Author

dhermes commented Jan 20, 2016

@tseaver Bump

@dhermes
Copy link
Contributor Author

dhermes commented Jan 28, 2016

@tseaver PTAL

@tseaver
Copy link
Contributor

tseaver commented Jan 29, 2016

I'm still against stripping the system tests from the sdist. Can we move the importable support code into a subpackage of gcloud, and just keep system_tests off the PYTHONPATH?

@dhermes
Copy link
Contributor Author

dhermes commented Jan 29, 2016

Why do we want to ship our system tests?

@tseaver
Copy link
Contributor

tseaver commented Jan 29, 2016

So that downstream packagers (e.g., .deb/.rpm) can run them to verify they have working code?

@dhermes
Copy link
Contributor Author

dhermes commented Jan 29, 2016

Our system tests? These require a ton of set-up, including having to go to the Cloud Console and download a key.

@tseaver
Copy link
Contributor

tseaver commented Jan 29, 2016

I've gotten feedback from .deb/.rpm maintainers in the past that they wanted to be able to verify the working state of the code they package by running tests afterward. Maybe they wouldn't run the system tests.

OTOH, as the APIs change over time, asking users to run them to help debug configuration issues vs. software might be reasonable.

@dhermes
Copy link
Contributor Author

dhermes commented Jan 29, 2016

It's not like git clone is difficult for the .deb/.rpm maintainers, or for users that are able to offer useful help

@dhermes
Copy link
Contributor Author

dhermes commented Feb 3, 2016

@tseaver WDYT of my comment above?

@theacodes
Copy link
Contributor

LGTM, on the basis that gcloud isn't the type of library we would expect to see as an OS-level dependency. Nor do I think it's a good idea to pursue that path, as it would hold our releases and support subject to OS release cycles (which are generally very slow).

@dhermes
Copy link
Contributor Author

dhermes commented Feb 12, 2016

Filed #1451 for discussion of the possibility to ship the system tests as gcloud.system_tests. Merging for now.

dhermes added a commit that referenced this pull request Feb 12, 2016
Excluding system tests from being installed.
@dhermes dhermes merged commit 62ba7c5 into googleapis:master Feb 12, 2016
@dhermes dhermes deleted the exclude-sys-tests branch February 12, 2016 20:22
dhermes added a commit to dhermes/google-cloud-python that referenced this pull request Feb 13, 2016
dhermes added a commit to dhermes/google-cloud-python that referenced this pull request Feb 19, 2016
Also removing system_tests/__init__.py so it is no longer
a package and making all imports happen locally (rather
than from the root of the project).

Changes originally inspired by emulator script breakages in googleapis#1373.
dhermes added a commit to dhermes/google-cloud-python that referenced this pull request Feb 19, 2016
Also removing system_tests/__init__.py so it is no longer
a package and making all imports happen locally (rather
than from the root of the project).

Changes originally inspired by emulator script breakages in googleapis#1373.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement. packaging testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants