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

Modified the setup-ember-dev test helper to use ember-metal/debugs override hooks #4260

Merged
merged 1 commit into from
Mar 25, 2016

Conversation

workmanw
Copy link

I've been helping to "addonify"® ember-data-model-fragments which also uses the ember-dev test suite. I noticed that expectNoDeprecations was no longer working as expected. After investigating I discovered that because Ember no longer uses Ember.deprecate internally, overriding that has little effect. I checked to see how ember-data's test suite handled this and discovered you have the same problem. See: setup-ember-dev.js#L7-L13.

This PR fixes that issue by using ember-metal/debug proper hooks. This PR is meant to keep backwards compatibility and work with older versions of ember which did not offer these hooks. If the feeling is that ember and ember-data usage is truly lockstep, this PR could be simplified by removing the getDebugFunction and setDebugFunction all together. Example: ember-data-model-fragments/.../setup-ember-dev.js.

Lastly, I ran the tests locally and there were quite a few failures due to deprecations spewing out. This PR was intended to start a discussion about that. Temporary insanity 😜 .

@bmac
Copy link
Member

bmac commented Mar 23, 2016

This is great @workmanw! What are the next steps? Do you want some help fixing the failures due to deprecations?

const AVAILABLE_ASSERTIONS = ['expectAssertion', 'expectDeprecation', 'expectNoDeprecation', 'expectWarning', 'expectNoWarning'];

function getDebugFunction(name) {
return Ember[name];
if (debugModule && debugModule.getDebugFunction) {
return debugModule.getDebugFunction();
Copy link
Member

Choose a reason for hiding this comment

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

Is this missing the name being passed to getDebugFunction?

Copy link
Member

Choose a reason for hiding this comment

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

Ya, I think so...

Copy link
Author

Choose a reason for hiding this comment

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

Ooops! Good catch. I'll fix that.

@workmanw
Copy link
Author

@bmac I guess I'm not completely sure about what the next steps should be. Maybe we should catalog the deprecations and see how easy they are to solve.

@workmanw workmanw force-pushed the setup-ember-dev-use-metal-hooks branch 2 times, most recently from 46691f1 to ba15b84 Compare March 23, 2016 18:55
@pangratz
Copy link
Member

Basically I am 💯 👍 on this. My initial implementation was never the way to go anyway and using ember-meta/debug is definitely the way to go. Thanks for this nicer solution!!

Lastly, I ran the tests locally and there were quite a few failures due to deprecations spewing out. This PR was intended to start a discussion about that.

I am not sure I follow completely: if I run the code in this PR locally via ember try:testall, all scenarios are working and no uncaught deprecations are causing test failures (well, to be exact, I had to update this line and add a check that the ember-metal/debug module is available before requiring it; it looks like this is needed because this module is not available in the ember 1.13 scenario).

@workmanw
Copy link
Author

@pangratz Yea ... you're absolutely right. I swear there were test failures last night, but they're all gone today. ¯_(ツ)_/¯.

I'll push up a change to get ember 1.13 working.

@workmanw workmanw force-pushed the setup-ember-dev-use-metal-hooks branch from ba15b84 to fd9dc9e Compare March 23, 2016 21:13
…override hooks. Ember no longer uses `Ember.deprecate` internally, so overriding that has little effect.
@workmanw workmanw force-pushed the setup-ember-dev-use-metal-hooks branch from fd9dc9e to 2105fc3 Compare March 23, 2016 21:27
@workmanw
Copy link
Author

@bmac @pangratz Sorry for the confusion on my part here. I think this commit is actually good to go. No rush or anything. I just wanted to be clear that there actually aren't any deprecations. Just my craziness.

@bmac bmac merged commit eece6b6 into emberjs:master Mar 25, 2016
@bmac
Copy link
Member

bmac commented Mar 25, 2016

Thanks @workmanw

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.

4 participants