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 Warnings in Test Suite Output #976

Closed
ethagnawl opened this issue Oct 12, 2017 · 20 comments
Closed

Fix Warnings in Test Suite Output #976

ethagnawl opened this issue Oct 12, 2017 · 20 comments
Labels

Comments

@ethagnawl
Copy link
Contributor

While running the test suite (using Ruby 2.2.5 and 2.4.0) there are hundreds/thousands of lines worth of warnings output.

Some examples:

devise_token_auth/test/controllers/devise_token_auth/omniauth_callbacks_controller_test.rb:21:in `get_parsed_data_json': warning: URI.une
scape is obsolete

devise_token_auth/app/controllers/devise_token_auth/concerns/set_user_by_token.rb:81: warning: instance variable @resource not initialize
d

devise_token_auth/app/controllers/devise_token_auth/concerns/set_user_by_token.rb:81: warning: instance variable @resource not initialize
d

bundler/gems/omniauth-facebook-19634473820d/lib/omniauth/strategies/facebook.rb:79: warning: instance variable @authorization
_code_from_signed_request_in_cookie not initialized

They distract from the task at hand and, where possible, should be addressed.

screenshot 2017-10-12 11 18 40

@zachfeldman
Copy link
Contributor

@ethagnawl agreed, this is not helping when running tests. Would you have time to put together a PR that addresses this, perhaps? Thanks for bringing this to our attention!

@ethagnawl
Copy link
Contributor Author

@zachfeldman Sure. I can start chipping away at the infractions.

@zachfeldman
Copy link
Contributor

Urock!

@lynndylanhurley
Copy link
Owner

I'm working on this now. Some of these warnings are legit, but it looks like a lot of them are coming from outdated test deps. I'll post a PR in a bit.

@zachfeldman
Copy link
Contributor

Thanks @lynndylanhurley !

@MaicolBen
Copy link
Collaborator

Thanks for removing some of them! But still there are tons of warnings, the main ones are:

  • lib/devise/models/confirmable.rb:297: warning: instance variable @reconfirmation_required not initialized (from devise I guess)
  • warning: method redefined; because a lot of metaprograming, I don't know how to remove them
  • warning: loading in progress, circular require considered harmful because including require File.expand_path('../dummy/config/environment', __FILE__), also, I don't know how to remove them

You can omit them with RUBYOPT=W0 rake test

@gitcoinbot
Copy link

This issue now has a funding of 0.08 ETH (61.36 USD) attached to it.

  • If you would like to work on this issue you can claim it here.
  • If you've completed this issue and want to claim the bounty you can do so here
  • Questions? Get help on the Gitcoin Slack
  • $12091.15 more Funded OSS Work Available at: https://gitcoin.co/explorer

@lynndylanhurley
Copy link
Owner

A lot of these warnings are outside of our control. I've noticed that running rails test instead of rake test ignores most of these. We may want to update the CI script to use that instead.

asartalo added a commit to asartalo/devise_token_auth that referenced this issue Dec 22, 2017
@asartalo
Copy link
Contributor

@lynndylanhurley the test warnings can be hidden from the test output by adding $VERBOSE = nil on test/test_helper.rb or editing the Rakefile as I did in my pull request at 448c813

As to wether hiding the warnings is the best solution for this, I'm not entirely sure.

asartalo added a commit to asartalo/devise_token_auth that referenced this issue Dec 23, 2017
Fixes deprecation warnings in test runs when models have been
modified prior to locking in updating auth header.

Part of a fix for lynndylanhurley#976
asartalo added a commit to asartalo/devise_token_auth that referenced this issue Dec 23, 2017
zachfeldman pushed a commit that referenced this issue Dec 28, 2017
* Remove verbose warnings in test output

Fixes issue #976

* Ensure resource is pristine before locking

Fixes deprecation warnings in test runs when models have been
modified prior to locking in updating auth header.

Part of a fix for #976

* Suppress expected omniauth errors in tests

Fix for #976

* Refactoring resource pristine code

Code improvements for 2208ae3 noted in code climate
@zachfeldman
Copy link
Contributor

@asartalo please feel free to claim the bounty! Thanks for your help on this one.

@owocki
Copy link

owocki commented Jan 2, 2018

@asartalo @zachfeldman hi from gitcoin.co ! let me know if you need any help claiming the bounty

@owocki
Copy link

owocki commented Jan 11, 2018

@zachfeldman should i be closing the bounty for this? i see that you closed the issue already...

@zachfeldman
Copy link
Contributor

@owocki I'm waiting for @asartalo to claim it?

@owocki
Copy link

owocki commented Jan 11, 2018

oh ok i was confused bc the github issue is closed now but the bounty is still open 👌

@zachfeldman
Copy link
Contributor

Oh ok yeah I don't know what to do in your system, feel free to explain it to me better. If I close the bounty does @asartalo get it? Do I have to award it first?

@owocki
Copy link

owocki commented Jan 11, 2018

once @asartalo claims the bounty ( at https://gitcoin.co/funding/details?url=https://github.com/lynndylanhurley/devise_token_auth/issues/976 ), @gitcoinbot will comment on this thread. then there will be an 'accept' button on gitcoin you can use the pay it out directly to his ETH address :)

@vs77bb
Copy link

vs77bb commented Jan 17, 2018

@zachfeldman @asartalo @owocki I still see this one open on Gitcoin. Wayne, can you claim so no one else sees it as open? If you need any gas to make the claim from Metamask, let us know :)

https://gitcoin.co/funding/details?url=https://github.com/lynndylanhurley/devise_token_auth/issues/976

@vs77bb
Copy link

vs77bb commented Jan 30, 2018

@zachfeldman Alternative route... if you think @asartalo has done enough to be awarded this bounty, we can tip him for the amount and you can close this one out. Might be worth doing to close the loop. cc @owocki - What do you guys think?

@zachfeldman
Copy link
Contributor

@vs77bb frankly anything that stops the notifications on this I'm down for. This has been way more communication about this than is necessary or useful, as an unpaid maintainer....

@MaicolBen
Copy link
Collaborator

You should use gitcoin or email to contact him but don't use the issues for this kind of communication

Repository owner locked as resolved and limited conversation to collaborators Jan 30, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

8 participants