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

add test verifying that registering an unknown service raises an exception #14850

Merged

Conversation

GavinJoyce
Copy link
Member

@GavinJoyce GavinJoyce commented Jan 18, 2017

This now passes, but didn't in Ember 2.10. I git bisected the test and it passes since #14360

})
});

throws(() => {
Copy link
Member

Choose a reason for hiding this comment

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

Should use expectAssertion here (otherwise it will fail for prod builds).

Copy link
Member Author

@GavinJoyce GavinJoyce Jan 18, 2017

Choose a reason for hiding this comment

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

this seems to be an Error, not an assertion:

throw new Error(`Attempting to inject an unknown injection: '${fullName}'`);

Should this be an assertion?

Copy link
Member

Choose a reason for hiding this comment

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

That validateInjections method is only called from here:

runInDebug(() => {
// Ensure that all lazy injections are valid at instantiation time
if (!validationCache[fullName] && typeof factory._lazyInjections === 'function') {
lazyInjections = factory._lazyInjections();
lazyInjections = container.registry.normalizeInjectionsHash(lazyInjections);
container.registry.validateInjections(lazyInjections);
}
});

Which is inside of a runInDebug...

Copy link
Member Author

@GavinJoyce GavinJoyce Jan 18, 2017

Choose a reason for hiding this comment

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

thanks

Copy link
Member Author

@GavinJoyce GavinJoyce Jan 18, 2017

Choose a reason for hiding this comment

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

It's also called from here, which doesn't seem to be wrapped in a runInDebug:

container.registry.validateInjections(injections);

This is likely why I get test failures when using expectAssertion

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to wrap the validateInjections in a runInDebug and change the Error to an assert

@GavinJoyce GavinJoyce force-pushed the gj/test-exception-with-unknown-service branch 4 times, most recently from b921a70 to 1ae1255 Compare January 18, 2017 21:43
@GavinJoyce GavinJoyce force-pushed the gj/test-exception-with-unknown-service branch from 1ae1255 to ccbc56b Compare January 18, 2017 21:46
@GavinJoyce
Copy link
Member Author

@rwjblue I believe that this is ready to go (I think the failing CI is just sauce labs flakiness).

I've wrapped a container.registry.validateInjections(injections) in a runInDebug so we get a bonus performance improvement

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

LGTM, I restarted the sauce labs job.

@GavinJoyce
Copy link
Member Author

It's 🍏, thanks

@rwjblue rwjblue merged commit ca76dba into emberjs:master Jan 18, 2017
@GavinJoyce GavinJoyce deleted the gj/test-exception-with-unknown-service branch January 18, 2017 22:56
@spruce
Copy link

spruce commented Mar 2, 2017

Hey,
I just ran into this problem with my addon ember-rapid-forms. Where it is possible to to use i18n but not a requirement. But with this change it fails the tests. Is there a way to walk around this issue. Maybe even a better way to support it. I have no better idea. Could you help? See here: https://travis-ci.org/piceaTech/ember-rapid-forms/jobs/207164170#L593

@GavinJoyce
Copy link
Member Author

@spruce this should help you: RSSchermer/ember-multiselect-checkboxes#41

@rwjblue
Copy link
Member

rwjblue commented Mar 2, 2017

I submitted piceaTech/ember-rapid-forms#153 to fix at least one example scenario in ember-rapid-forms.

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