Skip to content
This repository has been archived by the owner on May 26, 2019. It is now read-only.

Fix for Issue #501 Update examples in "Testing Components" guide to use Integration Tests #774

Merged
merged 1 commit into from
Sep 25, 2015

Conversation

toddjordan
Copy link
Contributor

See issue #501

I redid the same examples using integration tests. I also got the examples working in a sample app to verify them. I also included some other newer practices such as pods and some more es6 mechanisms.

There's enough changes here where you might want to look at the split diff

});
```

<!---
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this was already commented I went ahead and deleted. I can work on bringing it back if we feel its important. Now that we use integration tests I don't really think there's going to be much different about nested components that cause this section. you'd just declare the components in the render template and assert the DOM

@michaelrkn
Copy link
Contributor

Thanks a bunch for picking this up, @toddjordan!


Let's assume we have a component with a `style` property that is updated
whenever the value for its `name` property changes. The `style` attribute of the
component is bound to its `style` property.

> You can follow along by generating your own component with `ember generate
> component pretty-color`.
> component pretty-color --pod`.
Copy link
Contributor

Choose a reason for hiding this comment

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

We're not going to encourage people to use the pod directory structure until it's format has settled down. Can you re-do the examples with the "regular" structure for now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely. I'll go back and update

@michaelrkn
Copy link
Contributor

@locks can you review as well?

@toddjordan
Copy link
Contributor Author

@michaelrkn I also have a new section on stubbing services using the example in my blog post here:

http://presentationtier.com/stubbing-services-in-emberjs-integration-tests/

I'd like to add a new section to "Testing Components" based on the example, with language consistent with the others. Should I add the section to this PR?

Here a PR within my guides fork with the added section: https://github.com/toddjordan/guides/pull/2/files

@michaelrkn
Copy link
Contributor

@toddjordan I'd say open a separate PR for that, after this is merged. It's easier and faster to review things individually.

@locks Can you review this so that @toddjordan can move forward?

Thanks!

@toddjordan
Copy link
Contributor Author

will do, thx

@mixonic
Copy link
Member

mixonic commented Sep 21, 2015

@toddjordan I'm concerned with this.container.lookup in your test. I don't believe the container is considered public API. The rest seems reasonable, and there are ways to set values after lookup without using the container.

  • Stash the instance during init of the stub and set against that
  • Register a singleton object instead of a class for the service- may require some mucking about with the options for registering.

Nice writeup though!

@toddjordan
Copy link
Contributor Author

Thx for the tip. I'll look into modifying the example to a lookup alternative.

@mixonic
Copy link
Member

mixonic commented Sep 23, 2015

@toddjordan Also note that Ember 2.1 has tweaked some of these APIs. You can find links to data about those changes in this issue.

@toddjordan
Copy link
Contributor Author

I noticed the PR where you can do the same thing in a test with this.registry.register(...) as you can with this.container.register(...) If that's the preferred way I can update that.

Minor edits before PR

Update integration test paths that were wrong

Updates from initial PR comments

update after ember suave run
@michaelrkn
Copy link
Contributor

@toddjordan is this ready to merge? I don't see this.container.lookup, this.registry.register, or this.container.register in there, so I want to make sure it's in the right place, since that business is a bit above my head :)

@toddjordan
Copy link
Contributor Author

@michaelrkn yeah that discussion was around another PR for the new section on stubbing services for component integration tests. I haven't PR'd that to guides yet. This PR is just converting this existing examples, so yes, its good to go when you guys are ok with it.

michaelrkn added a commit that referenced this pull request Sep 25, 2015
Update examples in "Testing Components" guide to use Integration Tests
@michaelrkn michaelrkn merged commit ebaf4ed into emberjs:master Sep 25, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants