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 tests as specified in #14 #23

Merged
merged 1 commit into from
Dec 2, 2014

Conversation

pogopaule
Copy link
Contributor

This PR adds tests for controller, route, model and component; the places t gets injected in so far.

Note that this PR also fixes a bug. T didn't get injected in model.

@@ -125,7 +125,7 @@ export default Ember.Object.extend({
export default DS.Model.extend({
name: function() {
return this.t('name', 'John', 'Doe');
}
}.property()
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to have this as a separate commit as it does not fit within the context of this PR.

In addition, we prefer the long-form for CPs:

name: Ember.computed(function() {
   return this.t('name', 'John', 'Doe');
})

@pogopaule
Copy link
Contributor Author

alright will change the PR according to your comments

@pogopaule
Copy link
Contributor Author

I tried to rewrite the tests. Basically I wanted to implement the initializer unit test. The problem: I am not able to join the ember-wires correctly. My idea was to write something like that. But unfortunately t does not get injected. So, can you give me a hint on how to do this? I can not find any working example of an initializer test on github...

[...]
module('TInitializer', {
  setup: function() {
    Ember.run(function() {
      container = new Ember.Container();
      application = Ember.Application.create();
      application.deferReadiness();
    });
  }
});

test('it works', function() {
  initialize(container, application);

  var FooController = Ember.Controller.extend({
    foo: Ember.computed(function() {
      return this.t('foo');
    });
  });

// register FooController in container?

  var fooInstance = container.?????????() // the magic method that creates an instance of FooController where t gets injected according to the initializer.
  equal(fooInstance.get('foo'), 'bar');
});

@rwjblue
Copy link
Contributor

rwjblue commented Dec 2, 2014

@pogopaule - You would need to create that controller subclass via registration/lookup in the container.

Something like:

test('it works', function() {
  initialize(container, application);

  container.register('controller:foo', Ember.Controller.extend({
    foo: Ember.computed(function() {
      return this.t('foo');
    });
  }));

  var fooInstance = container.lookup('controller:foo');
  equal(fooInstance.get('foo'), 'bar');
});

@pogopaule
Copy link
Contributor Author

@rwjblue: Thanks for your quick reply! That is what I already tried. Forgot to mention this properly. Sorry for that!

I figured out that the problem is in these two lines. container and application.__container__ are two distinct objects. utils:t lives in application.__container__ whereas controller:foo lives in container. They don't know each other...

So is this a mistake in the blueprint or is there a good reason for a separate container? Shouldn't this rather be container = application.__container__ one line below?

@rwjblue
Copy link
Contributor

rwjblue commented Dec 2, 2014

Yes, it is a bug (I submitted a PR for it a few days back: ember-cli/ember-cli#2582).

@pogopaule
Copy link
Contributor Author

Ok, I guess #14 is done now. What do you think? Shall I squash the commits?

@bcardarella
Copy link
Contributor

nice :) Yes, please squash

- Adds tests for controllers, routes, models and components
- Fixes bug: In the initializer there is a typo that prevents t from being injected
into models. This bug needs to be fixed, otherwise the test for the
models fails.

 #14
@pogopaule
Copy link
Contributor Author

Done

bcardarella added a commit that referenced this pull request Dec 2, 2014
Add tests as specified in #14
@bcardarella bcardarella merged commit 0b00605 into DavyJonesLocker:master Dec 2, 2014
@rwjblue
Copy link
Contributor

rwjblue commented Dec 2, 2014

Awesome, thank you!

@pogopaule
Copy link
Contributor Author

Thanks for the merge. Glad I could help. And thank you for the project. Helps me a lot :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants