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

Adding standalone integration test support #21

Merged
merged 4 commits into from
Mar 9, 2015

Conversation

ef4
Copy link
Contributor

@ef4 ef4 commented Mar 9, 2015

This adds a new kind of test module that fills a gap in the current set of possible tests. So far we have:

  • unit tests that are good for testing algorithmic correctness in isolation.
  • acceptance tests that are good for testing within a complete application.

But we're missing the ability to integration test a unit smaller than whole-application. This PR adds a new TestModuleForIntegration that lets you drive an integration test with a template. I think this is often a more appropriate way to test Ember Components than a unit test, because interesting components tend to have rich dependence on other components.

The included tests illustrate the basic use cases: rendering a template, setting values in its context, and handling its events.

self.view = Ember.View.create({
context: self,
controller: self,
template: Ember.Handlebars.compile(templateString),
Copy link
Member

Choose a reason for hiding this comment

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

We should allow for a precompiled template to be passed in. I can't recall if Ember.Handlebars.compile properly handles a precompiled template...

@rwjblue
Copy link
Member

rwjblue commented Mar 9, 2015

The view created in this.render needs to be destroyed properly (likely via adding to teardownSteps).

@rwjblue
Copy link
Member

rwjblue commented Mar 9, 2015

I'd like to make https://github.com/switchfly/ember-test-helpers/blob/master/lib/ember-test-helpers/test-module.js#L99-L101, create the container as you have here if a integration: true parameter was passed in callbacks. Then this becomes test-module-for-snippet or something..

Thoughts?

@ef4
Copy link
Contributor Author

ef4 commented Mar 9, 2015

There is a bad assumption baked into the existing nomenclature, which is that components should get unit tests and not integration tests. I think that is often the wrong default choice, which is why people keep hitting frustration around needs.

And I don't think just having an integration: true flag on the existing unit test API is ideal, because that whole API is designed around unit testing, and you can write much nicer integration tests with an API (like this PR) that is designed specifically for integration tests.

Just to pick one example, consider block param handling:

this.render(`
{{#my-component as |value|}}
  <div class="sample">{{value}}</div>
{{/my-component}}
`);
equal(this.$('.sample').text(), 'correctValue');

I'm not even sure how you'd write that test using moduleForComponent, and that has nothing to do with needing access to a non-isolated container. From an API standpoint, the biggest problem is that users should almost never need to actually handle a Component instance in the usual course of writing an Ember app. Their tests should work the same way.

I'm not a fan of moduleForSnippet. A single module may test many different snippets, one per test. If we're looking for better names, I'd actually prefer componentIntegrationTest and componentUnitTest to replace moduleForComponent, etc.

@rwjblue
Copy link
Member

rwjblue commented Mar 9, 2015

And I don't think just having an integration: true flag on the existing unit test API is ideal

Nothing about the current API makes it unit testing, other than that is what we have generally called it. Specifying needs: ['type:thing', 'other-type:other-thing'] inherently makes it an integration test (with a stupidly annoying and brittle hard coding of collaborators). There are many parts of the system that cannot be tested via a template snippet (routes, services, models, etc) and they would all benefit from an integration: true flag to become less brittle. We should be encouraging people to break apart logic into easier tested pieces (which means services, utils, etc) and the current setup basically makes all of the tests for a specific module fail as soon as you add a collaborator that is looked up via the container.

tldr;

I am not suggesting that adding integration: true replace the functionality you have added in TestModuleForIntegration, I am asking that we move the setupContainer logic in TestModuleForIntegration (whether or not to use an isolated container or a "real" one that is hooked up to the resolver) into the main TestModule so that other non-snippet tests can become less brittle. Then in TestModuleForIntegration the integration flag is set to true before calling this._super here.

@ef4
Copy link
Contributor Author

ef4 commented Mar 9, 2015

That sounds fine. We can merge this and then refactor the non-isolated container into a shared place.

I agree that the existing moduleFor with a non-isolated container is needed for services & models. I would still favor replacing the existing moduleForComponent with this, so we're not leading newbies down the wrong path. But that decision does not need to be made now. We can ship this new option and see how it's adopted.

@rwjblue
Copy link
Member

rwjblue commented Mar 9, 2015

I would still favor replacing the existing moduleForComponent with this, so we're not leading newbies down the wrong path.

I agree 100%.

@rwjblue
Copy link
Member

rwjblue commented Mar 9, 2015

I'm 👍 on landing this now, and refactoring the isolated container bits into TestModule next.

@dgeb - Can you review and 👍 / 👎 also?

@dgeb
Copy link
Member

dgeb commented Mar 9, 2015

@ef4 thanks for this! I agree that an integration testing module has been sorely missing. I am 👍 on merging.

Integration vs. unit tests don't have to be an either / or decision: I believe both should be created by model and component generators.

@stefanpenner
Copy link
Member

love it.

rwjblue added a commit that referenced this pull request Mar 9, 2015
Adding standalone integration test support
@rwjblue rwjblue merged commit 17bb72a into emberjs:master Mar 9, 2015
@ef4
Copy link
Contributor Author

ef4 commented Mar 10, 2015

Once we have a release here I will submit PRs to ember-qunit and ember-mocha.

@jamesarosen
Copy link

Some ideas:

What if the generated skeleton was for a unit test, but told you how to "upgrade" to an integration test?

// If you want access to integration test helpers like `click`, change
// `moduleForComponent` to `integrationModuleForComponent`.
moduleForComponent('foo-bar', 'FooBarComponent');

test('it exists', function(assert) {
  assert.ok(true);
});

Alternatively, what if the install integration test helpers action was performed like the current starApp?

moduleForComponent('foo-bar', 'FooBarComponent', {
  setup: function() {
    // Uncomment if you need integration test helpers like click()
    // this.needsIntegrationTestHelpers();
  }
});

Lastly, a related, but different question: how would this.render work without a Handlebars compiler? As of Ember-CLI 0.2.0, it's not part of the runtime, even in test mode. Should we submit a patch to ember-cli that adds ember-template-compiler to the app in test mode by default so it's easier to do view-layer testing?

@ef4
Copy link
Contributor Author

ef4 commented Mar 12, 2015

Lastly, a related, but different question: how would this.render work without a Handlebars compiler? As of Ember-CLI 0.2.0, it's not part of the runtime, even in test mode. Should we submit a patch to ember-cli that adds ember-template-compiler to the app in test mode by default so it's easier to do view-layer testing?

This does require the template compiler. I think ember-cli should ship the compiler in the test support bundle by default, but with the important caveat that it should be available as module you can explicitly import, not as a global that app code can accidentally depend on, causing tests to pass that should have failed.

@rwjblue
Copy link
Member

rwjblue commented Mar 12, 2015

I think ember-cli should ship the compiler in the test support bundle by default, but with the important caveat that it should be available as module you can explicitly import, not as a global that app code can accidentally depend on, causing tests to pass that should have failed.

That caveat is king here. I've been burned by "accidentally" depending on the template compiler more than once. I'd prefer to get a good ES6 template string setup and still precompile in node-land.

I said more things here: ember-cli/ember-cli#3497 (comment)

@ef4
Copy link
Contributor Author

ef4 commented Mar 12, 2015

That would be nice. Seems like a good use of es6 tagged template strings.
On Mar 12, 2015 1:28 PM, "Robert Jackson" [email protected] wrote:

I think ember-cli should ship the compiler in the test support bundle by
default, but with the important caveat that it should be available as
module you can explicitly import, not as a global that app code can
accidentally depend on, causing tests to pass that should have failed.

That caveat is king here. I'd prefer to get a good ES6 template string
setup and still precompile in node-land.

I said more things here: ember-cli/ember-cli#3497 (comment)
ember-cli/ember-cli#3497 (comment)


Reply to this email directly or view it on GitHub
#21 (comment)
.

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.

5 participants