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

Skipping system tests when credentials env. var is unset. #3475

Merged
merged 1 commit into from
Jun 29, 2017
Merged

Skipping system tests when credentials env. var is unset. #3475

merged 1 commit into from
Jun 29, 2017

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Jun 5, 2017

This is labeled "don't merge" because it is wrong in it's current state.

So @lukesneeringer @tseaver @jonparrott WDYT the "correct" form should be? We don't want it to raise on PRs, we want it to gracefully do nothing. Otherwise, this is a helpful message and status for people running tests locally with a "bad" setup.

@dhermes dhermes added build do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Jun 5, 2017
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 5, 2017
duggelz
duggelz reviewed Jun 6, 2017
View reviewed changes
bigquery/nox.py Outdated
@@ -49,7 +50,8 @@ def system_tests(session, python_version):

# Sanity check: Only run system tests if the environment variable is set.
if not os.environ.get('GOOGLE_APPLICATION_CREDENTIALS', ''):
return
raise nox.command.CommandFailed(
reason='Credentials must be set via environment variable.')

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@lukesneeringer
Copy link
Contributor

lukesneeringer commented Jun 13, 2017 •
edited
Loading

Thoughts on this (sorry for the delay):

  • It is totally possible to read one of the CircleCI / AppVeyor pull request variables and using those as the basis to skip. (I am less convinced it is a good idea.)
  • I am not as convinced as @dhermes that you always want to run system tests in local testing. In particular, I would expect external contributors generally do not want to do so, or want to run them for only the package they are working on.
  • We could add some kind of skipped state to nox. It would be far more valuable for us to report "5 sessions passed, 2 skipped" than "7 sessions passed". I think that would be my preference. It also follows @jonparrott's desire to "generally follow py.test as much as possible" as this is definitely a well-understood testing feature. It is also a useful feature lacking in tox. :-)

@theacodes
Copy link
Contributor

theacodes commented Jun 13, 2017 via email •
edited by lukesneeringer
Loading

@lukesneeringer
Copy link
Contributor

@dhermes Does that work for you?

@dhermes
Copy link
Contributor Author

dhermes commented Jun 13, 2017

Yes I am OK with that. I don't have bandwidth to add that feature to nox though

@theacodes
Copy link
Contributor

theacodes commented Jun 13, 2017 via email •
edited by dhermes
Loading

@dhermes dhermes mentioned this pull request Jun 13, 2017
Closed
@dhermes
Copy link
Contributor Author

dhermes commented Jun 13, 2017

Donezors: wntrblm/nox#34

@dhermes dhermes force-pushed the fail-tests-without-creds branch from 6410981 to ab0bc35 Compare June 28, 2017 17:23
@dhermes dhermes removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jun 28, 2017
@dhermes
Copy link
Contributor Author

dhermes commented Jun 28, 2017

I just removed the "do not merge" label.

@jonparrott I think this is read to go. PTAL?

@lukesneeringer ISTM that we'll need our CircleCI image to have the latest version of nox-automation, what's the best way to go about that?

@duggelz I have intentionally avoided linked to CONTRIBUTING.rst for the time being (though I think it's a great idea). This is because the doc is months out-of-date.

@dhermes
Copy link
Contributor Author

dhermes commented Jun 28, 2017

Update: currently waiting on @lukesneeringer to rebuild our Docker image, so we can ditch the pip install --upgrade nox-automation that I added.

(It's not a big hurry to merge this.)

@dhermes dhermes changed the title Failing system tests when credentials env. var is unset. Skipping system tests when credentials env. var is unset. Jun 28, 2017
@dhermes dhermes force-pushed the fail-tests-without-creds branch from 6519b5c to 2685c36 Compare June 29, 2017 17:19
@dhermes
Copy link
Contributor Author

dhermes commented Jun 29, 2017

OK @lukesneeringer got us googleapis/nox:0.17.0 so this thing is ready to go once green.

@dhermes dhermes mentioned this pull request Jun 29, 2017
Merged
@dhermes dhermes merged commit b9cb6d1 into googleapis:master Jun 29, 2017
@dhermes dhermes deleted the fail-tests-without-creds branch June 29, 2017 17:56
tseaver pushed a commit that referenced this pull request Jul 10, 2017
@dhermes @tseaver
Skipping system tests when credentials env. var is unset. (#3475)
45b32b6
@dhermes dhermes mentioned this pull request Jul 14, 2017
Merged
@lukesneeringer lukesneeringer mentioned this pull request Aug 11, 2017
Closed
landrito pushed a commit to landrito/google-cloud-python that referenced this pull request Aug 21, 2017
@dhermes @landrito
Skipping system tests when credentials env. var is unset. (googleapis…
a6154a3 
…#3475)
landrito pushed a commit to landrito/google-cloud-python that referenced this pull request Aug 22, 2017
@dhermes @landrito
Skipping system tests when credentials env. var is unset. (googleapis…
9c2b37b 
…#3475)
landrito pushed a commit to landrito/google-cloud-python that referenced this pull request Aug 22, 2017
@dhermes @landrito
Skipping system tests when credentials env. var is unset. (googleapis…
262f55a 
…#3475)
atulep pushed a commit that referenced this pull request Apr 3, 2023
@dhermes
Skipping system tests when credentials env. var is unset. (#3475)
bead634
atulep pushed a commit that referenced this pull request Apr 18, 2023
@dhermes
Skipping system tests when credentials env. var is unset. (#3475)
ccc74bb
parthea pushed a commit that referenced this pull request Jul 6, 2023
@dhermes
Skipping system tests when credentials env. var is unset. (#3475)
eaabe16
parthea pushed a commit that referenced this pull request Oct 21, 2023
@dhermes
Skipping system tests when credentials env. var is unset. (#3475)
4f0a678
parthea pushed a commit that referenced this pull request Oct 21, 2023
@dhermes
Skipping system tests when credentials env. var is unset. (#3475)
0916c9a
parthea pushed a commit that referenced this pull request Oct 21, 2023
@dhermes
Skipping system tests when credentials env. var is unset. (#3475)
fcfdf18
parthea pushed a commit that referenced this pull request Oct 22, 2023
@dhermes
Skipping system tests when credentials env. var is unset. (#3475)
b6aa9db
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@duggelz duggelz duggelz left review comments

@theacodes theacodes theacodes approved these changes

@tseaver tseaver tseaver approved these changes

Assignees

@lukesneeringer lukesneeringer

@tseaver tseaver

@theacodes theacodes

Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dhermes @lukesneeringer @theacodes @tseaver @duggelz @googlebot