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

[Glimmer 2] Remove tests related to legacy ember-test-helpers #13671

Conversation

chadhietala
Copy link
Contributor

@chadhietala chadhietala commented Jun 14, 2016

These tests are specifically testing patterns that used to be used in
ember-test-helpers. Since these tests have been introduced
ember-test-helpers has moved to the container pattern with
lookupFactory. In general we need to start migrating tests away from
things that did not come out of the container with injections on them.

This is because we need to remove this hardcoded InteractiveRenderer
here

Part of #13644

These tests are specifically testing patterns that used to be used in
ember-test-helpers. Since these tests have been introduced
ember-test-helpers has moved to the container pattern with
`lookupFactory`. In general we need to start migrating tests away from
things that did not come out of the container with injections on them.

This is because we need to remove this hardcoded `InteractiveRenderer`
[here](https://github.com/emberjs/ember.js/blob/master/packages/ember-views/lib/views/core_view.js#L46-L52)
@chadhietala chadhietala force-pushed the nuke-link-to-tests-related-to-qunit branch from b0b1de7 to 1e2aed4 Compare June 14, 2016 23:04
@chadhietala
Copy link
Contributor Author

@rwjblue Can you confirm this is ok?

@rwjblue
Copy link
Member

rwjblue commented Jun 15, 2016

I'd like to keep the tests, but we need to build the registry properly. The issue here is that these tests do not use Ember.Application.buildRegistry to create the registry instance. They passed before because there were no services that were used that weren't setup, but they fail in glimmer because service:-document isn't present and the Environment requires it.

@chadhietala
Copy link
Contributor Author

Yea I walked through the code to figure out why it was failing. Do you know of other tests that go through buildRegistry? I would just move these to the new build infra, but these tests are sort of in an in between state of what the new infra supports.

@rwjblue
Copy link
Member

rwjblue commented Jun 16, 2016

I think moving them to ember package may be a better bet.

@rwjblue
Copy link
Member

rwjblue commented Jun 18, 2016

Rebased in #13713

@rwjblue rwjblue closed this Jun 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants