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

Silence warnings and deprecations in the console during tests #4663

Merged
merged 2 commits into from
Nov 30, 2016

Conversation

bmac
Copy link
Member

@bmac bmac commented Nov 17, 2016

Closes #4416

Before

screen shot 2016-11-17 at 10 04 57 am

screen shot 2016-11-17 at 10 05 05 am

After

screen shot 2016-11-17 at 9 57 12 am

@stefanpenner
Copy link
Member

just checking but this doesn't only silence them, but asserts they do or do not happen & happens to silence them right?

Copy link
Member

@pangratz pangratz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly I am not super happy about ignoring the deprecations. I think the desired approach here would be to expect a warning in every test which actually uses "to be deprecated" behavior.

Or am I missing something?

Basically ❤️ 👍 for making the test output less noisy! Thanks for working on this!

@@ -836,7 +836,7 @@ test("the promise returned by `_scheduleFetch`, when it rejects, does not depend
});

test("store._fetchRecord reject records that were not found, even when those requests were coalesced with records that were found", function(assert) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above: s/test/testInDebug

@bmac
Copy link
Member Author

bmac commented Nov 17, 2016

just checking but this doesn't only silence them, but asserts they do or do not happen & happens to silence them right?

I added 2 assertions that happens to silence the warnings mainly for the side effect of silencing the warnings. The json-serializer-test.js did not have an existing test for the warning, however the adapter-interop-test does have an explicit test for the warning which my change now duplicates.

For the since almost all of the tests in the errors object trigger a warning I overrode Ember.warn for that entire module.

I also added an ignoreDeprecation in the rest-adapter-test.js which doesn't and any assertions since there is already a test for that deprecation

@stefanpenner
Copy link
Member

Although a pain, I think the ideal is. error on unexpected warn, error on unexpected deprecation. And require assertions in those places.

Maybe there is a middleground, but hiding warning/deprecations is a slippery slope.

@pangratz
Copy link
Member

We had this once (added in #3834) but somehow this isn't working anymore. At least not the last time I checked (if I remember correctly).

@pangratz
Copy link
Member

I see that there has been some work in #3995.

@bmac do you want to investigate if this might be working now?

@bmac
Copy link
Member Author

bmac commented Nov 18, 2016

I have updated this pr to explicitly tests for all of the deprecations and warnings that were ignored in the earlier version of this pr.

@pangratz enabling that flag now causes the build to fail when there is a deprecation warning.

@@ -2,7 +2,7 @@
import Ember from 'ember';
import EmberTestHelpers from "ember-dev/test-helper/index";

const AVAILABLE_ASSERTIONS = ['expectAssertion', 'expectDeprecation', 'expectNoDeprecation', 'expectWarning', 'expectNoWarning'];
const AVAILABLE_ASSERTIONS = ['expectAssertion', 'expectDeprecation', 'expectNoDeprecation', 'expectWarning', 'expectNoWarning', 'ignoreDeprecation'];
Copy link
Member

@pangratz pangratz Nov 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like ignoreDeprecation is no more used?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, nevermind. I think it doesn't hurt if this assertion is available too.

@pangratz pangratz merged commit 9861ae6 into emberjs:master Nov 30, 2016
@pangratz
Copy link
Member

Thanks for your work in this @bmac!! 🚀

@bmac bmac deleted the issue-4416 branch November 30, 2016 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants