Remove verbose warnings in test output #1049
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks! |
|
Hey @asartalo thanks for your PR. Can you paste a screenshot of how many warnings are showing now? This is a really small commit so if we're going to a pay a bounty on this ticket, we'd want to ensure this is really removing all warnings when running the suite and not just hiding them. |
|
@zachfeldman we can just check the travis output. Looks like there are still a couple of outstanding issues. In this case it's logging an omniauth failure when we are testing the handling of the failure: It would be nice to silence the omniauth warnings. The other warning is an actual issue that we should fix: Should be an easy fix. |
|
Thanks @lynndylanhurley ! @asartalo I'd use that comment as a guideline for how to proceed. |
|
Thanks! I actually got here through the bounty system and it's my first time. After finding out how to go about this, I didn't know if I should even claim the bounty because the fix may have been too small. I'll work on it right away. |
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
Code improvements for 2208ae3 noted in code climate
|
There should no longer be unnecessary messages after this. Should I squash all commits into 1? |
I think the commits look great. Thanks @asartalo! |
|
@asartalo much better! If we remove silencing warnings though, are there still a lot of warnings? |
|
@asartalo to clarify....does the silencing line silence any legit warnings? Would there be ways to remove those warnings entirely rather than just silencing them? |
|
If we remove the We can probably work on the devise_token_auth side but the rest are on the included libraries. |
|
Thanks @asartalo ! @lynndylanhurley what do you think of these warnings? |
|
It looks like they should all be ignored. I'll just go through some of the different types: ^ this is a code-smell thing that should be fixed, but it's in another gem and completely outside of our control. There are several issues like this that are caused by external dependencies. ^ a method was redefined (intentionally). There are a ton of these warnings, and in each case it's intentional. ^ an instance variable was not initialized before it was used. The result is that it evaluates to ^ I've looked into this, and from what I can tell this warning is due to the method not following some kind of spec. It seems like this will eventually be resolved internally by the And from what I can tell that's it. These warnings seem like they should be ignored. Am I missing anything? |
|
Nope sounds good! Gonna merge this @lynndylanhurley |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A fix for issue #976.
This removes most of the verbose warnings shown when running
rake test.