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

Fix a failing test introduced in #28. #35

Merged
merged 1 commit into from
Jul 10, 2015

Conversation

mr-salty
Copy link
Contributor

The environment variables in the 'fails if env vars are set' test
were not actually being set because they used a nonexistent hash key
(which has the effect of unsetting the environment variable).

The environment variables in the 'fails if env vars are set' test
were not actually being set because they used a nonexistent hash key
(which has the effect of unsetting the environment variable).
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 29, 2015
@mr-salty
Copy link
Contributor Author

@haabaato PTAL - this makes the test pass but let me know if there's a better fix.

mr-salty referenced this pull request Jun 29, 2015
- ServiceAccountCredentials, ServiceAccountJwtHeaderCredentials
and UserRefreshCredentials initializers now take keyword args
via options hash.

- In `credentials_loader.rb`, refactored env var checking into
private methods

- Updated tests & added new tests.

- Fixed existing test for #from_well_known_path 'fails if
the file is invalid', where `from_env` was called instead of
`from_well_known_path`.

- Fixed rubocop errors I introduced, but two existing ones remain.

- Added entry to changelog.

- Fixed rubocop errors from code containing parallel assignments

- Updated rubocop_todo.yml to ignore parallel assignments and
trailing underscore assignments.
@haabaato
Copy link

Apologies for the delay as I'm currently attending a conference. 👍
Seems like a simple fix. Test was previously passing because NOT_FOUND_ERROR was being raised.

@mr-salty
Copy link
Contributor Author

mr-salty commented Jul 1, 2015

this was addressed more properly in #36 so closing this.

@mr-salty mr-salty closed this Jul 1, 2015
@haabaato
Copy link

haabaato commented Jul 1, 2015

@mr-salty #36 doesn't address this issue. Your PR seems fine to me.

@mr-salty
Copy link
Contributor Author

mr-salty commented Jul 1, 2015

argh, sorry, I read that too fast.

@mr-salty mr-salty reopened this Jul 1, 2015
@tbetbetbe
Copy link
Contributor

Is this still necessary ?

@mr-salty
Copy link
Contributor Author

mr-salty commented Jul 7, 2015

The test still fails for me without this change, so I think it's still needed.

@tbetbetbe
Copy link
Contributor

Can you describe a bit more the environment where this test is failing; at the moment Travis is green ?

@tbetbetbe
Copy link
Contributor

Ok, this is safe to add as it does not break the tests, and IUC, it breaks your other PR.

tbetbetbe added a commit that referenced this pull request Jul 10, 2015
Fix a failing test introduced in #28.
@tbetbetbe tbetbetbe merged commit 4c05740 into googleapis:master Jul 10, 2015
@mr-salty
Copy link
Contributor Author

That is a good question about why it was failing for me locally but not failing in travis (which it clearly is not). I was using ruby 1.9 but that doesn't seem to be it, I also see the failure with 2.2. @haabaato - any thoughts?

FWIW, here is the (edited) log:

$ git checkout caed076711de9375772123560837975f118238fb
[...]
$ ruby --version
ruby 2.2.0p0 (2014-12-25 revision 49005) [x86_64-linux]
$ rvm --version
rvm 1.26.11 (latest) by Wayne E. Seguin <wayneeseguin@gmail.com>, Michal Papis <mpapis@gmail.com> [https://rvm.io/]
$ bundle --version
Bundler version 1.10.5
$ gem --version
2.4.6
$ bundle exec rake
[...]
Failures:

  1) #get_application_default when credential type is unknown fails if env vars are set
     Failure/Error: expect(&blk).to raise_error RuntimeError
       expected RuntimeError but nothing was raised
     # ./spec/googleauth/get_application_default_spec.rb:240:in `block (3 levels) in <top (required)>'

Finished in 6.48 seconds (files took 0.34353 seconds to load)
82 examples, 1 failure

Failed examples:

rspec ./spec/googleauth/get_application_default_spec.rb:234 # #get_application_default when credential type is unknown fails if env vars are set

@mr-salty mr-salty deleted the fix-env-var-test branch July 10, 2015 18:44
@haabaato
Copy link

The test was always passing for me locally, but as I was trying to mention above, it was only passing because NOT_FOUND_ERROR is raised in get_application_default: https://github.com/google/google-auth-library-ruby/blob/master/lib/googleauth.rb#L116.
Though I had written the test assuming that read_creds would raise the error here: https://github.com/google/google-auth-library-ruby/blob/master/lib/googleauth.rb#L77

@mr-salty
Copy link
Contributor Author

Ah, thanks - now I understand exactly what is happening. In my case it was picking up ~/.config/gcloud/application_default_credentials.json off the local disk and not throwing the exception. If I remove that file, then the test passes even without this PR.

Using fakefs everywhere (which Tim already opened as #40) would prevent that from happening. FWIW, I did try doing that (it's literally a 1-liner, 'require fakefs'), but there were some failures, so I went with a more narrow approach for my new tests. But, it's definitely a good idea. Maybe we can also do ENV.clear to prevent the user's environment from leaking in?

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants