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

[Glimmer2] Migrate local lookup tests #13345

Merged
merged 1 commit into from
Apr 19, 2016
Merged

Conversation

asakusuma
Copy link
Contributor

Not sure if this is the right way to simulate the resolver going forward.

@rwjblue
Copy link
Member

rwjblue commented Apr 15, 2016

Assigning to myself for review, will try to review tonight.

@rwjblue rwjblue self-assigned this Apr 15, 2016
@rwjblue
Copy link
Member

rwjblue commented Apr 16, 2016

I'm unsure what packages/container/tests/test-helpers/.Rhistory is (maybe an editor artifact?).

export default function buildOwner(props) {
let Owner = EmberObject.extend(RegistryProxy, ContainerProxy, {
init() {
this._super(...arguments);
let registry = new Registry(this._registryOptions);
let registry = new Registry(this._registryOptions || {
resolver: buildResolver()
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that all tests should get a local lookup enabled resolver. Especially since the actual patterns for how local lookup will work are still up in the air (see emberjs/rfcs#124). It seems better (to me) to inline this change, and the resolver above in the local-lookup-test file until those things are settled...

@rwjblue
Copy link
Member

rwjblue commented Apr 16, 2016

We should also update ApplicationTest's registerTemplate method in abstract-test-case.js to pass in the source.

@asakusuma
Copy link
Contributor Author

Got rid of packages/container/tests/test-helpers/.Rhistory

When you say pass in the source, are you talking about moduleName? I added that to registerTemplate

@rwjblue
Copy link
Member

rwjblue commented Apr 18, 2016

When you say pass in the source, are you talking about moduleName? I added that to registerTemplate

Yes, I meant moduleName, sorry about the confusion.

@rwjblue
Copy link
Member

rwjblue commented Apr 18, 2016

This is looking good, I see you still have it labeled as WIP though. Ping me when you are ready for another round of review...

@asakusuma asakusuma changed the title [WIP] [Glimmer2] Migrate local lookup tests [Glimmer2] Migrate local lookup tests Apr 18, 2016
@asakusuma
Copy link
Contributor Author

@rwjblue ok I removed the WIP as I have attempted to migrate all the local lookup tests now. Except for the one test that was for when ember-htmlbars-local-lookup=false, since the flag is true in features.json.

@rwjblue
Copy link
Member

rwjblue commented Apr 18, 2016

@asakusuma - I believe this file should be deleted and a symlink setup (to match the other sets of ember-glimmer / ember-htmlbars integration tests. Also, the commit still has [WIP] prefixed, can you remove that as well?

Let me know when ready for the next round of review...

@locks locks added the Glimmer2 label Apr 19, 2016
@asakusuma
Copy link
Contributor Author

@rwjblue ready for review. I deleted the old test, but the parent folder of the new test is already symlinked. [WIP] prefixed has been removed from commit.

@rwjblue
Copy link
Member

rwjblue commented Apr 19, 2016

Ahh, I see, I didn't realize the parent folder was symlinked. Thank you for pointing that out.

@rwjblue rwjblue merged commit fd52d75 into emberjs:master Apr 19, 2016
@rwjblue
Copy link
Member

rwjblue commented Apr 19, 2016

Thank you @asakusuma!

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.

3 participants