-
-
Notifications
You must be signed in to change notification settings - Fork 257
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
Support component integration tests #38
Conversation
this.container = isolatedContainer(this.needs); | ||
var isolated = isolatedContainer(this.needs); | ||
this.container = isolated.container; | ||
this.registry = isolated.registry; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes the isolated case consistent with the integrated case, which already provides this.registry
.
I have a few tiny comments, but the approach looks good to me. |
87b4972
to
0cec7a2
Compare
Based on feedback from @rwjblue I have made this a less jarring upgrade. For now we still default to unit test mode, but will fire a deprecation warning if you are in danger of getting the new behavior when we finally do switch the default. |
0cec7a2
to
6e19bd1
Compare
We can merge this to master when my PR lands and releases. emberjs/ember-test-helpers#38
LGTM. @dgeb - Any objections? |
I was never crazy about the name This is not just an issue with component integration tests. The I think I'd rather see us introduce integration test modules that provide the semantics to test actual integrations. Perhaps @ef4 - I really appreciate the hard work you've done to enable these integration capabilities. I would like to make sure we expose these capabilities in the most useful light to users via the higher level libs (ember-qunit and ember-mocha) and through the blueprints in ember-cli. |
I'm not 100% clear on what your position is on this. I realize "unit" and "integration" mean different things in different But it is clear that testing against public API is better than testing If we continue to generate component "unit" tests by default when people |
in general i like to define various testing levels the following way:
I believe components in ember, describe the JS/template that controls a chunk of template hierarchy. As such I do not believe today one can purely unit test this. It would have to be some adapted We should explore a future where this is possible, I believe we can improve it, but pragmatically speaking the path of least resistance today (and short term) is likely more permissive component testing helpers. This problem is shared by WebComponents and demonstrates that they aren't as isolated as people think. Their nested components are hidden, style and structure don't leak, but the implementation isn't actually encapsulated as at this phase, a concrete example we can build a discussion around: <my-cool-component>
<!-- public and external -->
</my-cool-component> <!-- my-cool-component.html -->
<my-internal-component />
<input/> <!-- my-internal-component.html -->
<my-other-internal-component /> When testing: We do have the flexibility today to provide a mock |
@ef4 Sorry I was not clear. I am not disagreeing, but seeking consensus on what we consider integration tests to be in ember. From a pragmatic standpoint, I agree that the kind of integration test in this PR should be the default for testing a component. As @stefanpenner points out, there is no way to purely unit test components in ember today. My point above is that there is room for a more general purpose layer of "integration" tests here. Integration tests that are focused on testing the interaction of multiple components together, multiple models together, or perhaps other aspects of an application instance. So really I'd like to come to an agreement on semantics to allow for space for general integration tests. I raised this here because this PR removes I agree with @stefanpenner's take on testing levels. So how do we translate those levels to these unit-ish integration tests of components? In which directory are these component test blueprints going to be placed? They seem to be the duck-billed platypus of testing. |
Ah, I understand better now. I guess we could still offer some more generic kind of integration test, I'm hesitant though. More kinds of tests pushes more choice burden onto In an Ember 2.0 world, everything is a component, so at every layer of your
|
I would like to help find consensus so we can land this feature. It sounds like everyone supports the idea of The two issues are separable. They got tied together because I eliminated So I propose merging the uncontroversial feature (integration-mode for components), instead of waiting until we can design this other possible thing (sandbox tests). I suspect that design will not be trivial, which is why |
return { | ||
container: container, | ||
registry: registry | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wary of changing the return value of isolatedContainer
here, especially given the likelihood of the container/registry reform work proceeding very soon (emberjs/rfcs#46). In other words, I don't want developers who are using isolatedContainer
to need to refactor twice in a short time. Maybe the question should be whether isolatedContainer
is really used outside of ember-mocha and ember-qunit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assumed that only ember-mocha and ember-qunit would be affected, but I could be wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can just implement isolatedRegistry
instead. isolatedContainer
can keep the same public API, but get reimplemented in terms of isolatedRegistry
.
(While also maintaining compatibility for old Embers that don't have a registry.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect! Thanks in advance @ef4 👍
@ef4 sorry for stalling on this. I am fine with merging once we resolve the |
No problem -- this is definitely an area where having a clear story is important. |
Does this include scaffolding the |
6e19bd1
to
c7cdf0a
Compare
Finally got around to making the last requested change, so that isolatedContainer maintains the same public API. This should be ready to go. |
@FlySwatter I don't think you actually need this PR to get the behavior you want in a model test. You should already be able to pass We do still need some clear documentation around that, and possibly some new blueprint generators. |
I suspect the That's one reason it's easier to stick with the naming conventions.
|
@ef4 great catch on the |
babel will likely start making it so that we can pre-compile easily via string templates: see: https://gist.github.com/stefanpenner/38a36298d04b29bbce5f This would allow us to do this, without having to include the pre-compiler. |
Support component integration tests
Thanks for all your work (and patience) @ef4! |
@stefanpenner - That gist looks nice, but still seems that it would use runtime compilation (not build time). Unless you are saying there are additional hooks added in babel. (we could of course write a babel transform that does this for us though) |
it would detect the string template handler, and pre-compile at build time. This allows valid language syntax to be used, but as an optimization (for both perf and dep size) it would be done at build time rather then runtime. |
@jamesarosen there does seem to be something wrong with the action handling. I would have expected your example to work. I will investigate further. |
I figured out the event handling problem. It's caused by doing ember-qunit does a fairly wacky thing at the start of every test that breaks event handling for any view created before that point. So you can't render in |
There is an existing ember-qunit issue about this behavior. https://github.com/rwjblue/ember-qunit/issues/86 |
I am all for this feature. I have borrowed @ef4 version from his liquid-fire repo and it is significantly better than the current unit tests for components. Not having initializers run in general, I understand, but there should be a clean way to spin up ember data in this environment. When components replace controllers, there needs to be a way to test the component/data interface without reimplementing Ember Data for each test. |
I agree so much! |
I played around with babel and created a // babel-htmlbars-compiler/index.js
var compiler = require('./ember-template-compiler');
module.exports = function(babel) {
var t = babel.types;
return new babel.Transformer('babel-htmlbars-precompile', {
TaggedTemplateExpression: function(node) {
if (t.isIdentifier(node.tag, { name: "hbs" })) {
var quasis = node.quasi.quasis;
var template = quasis.map(function(quasi) {
return quasi.value.cooked;
}).join("");
return "Ember.HTMLBars.template(" + compiler.precompile(template) + ")";
}
}
});
}; which in turn can be used within Ember-CLI: // Brocfile.js
var EmberApp = require('ember-cli/lib/broccoli/ember-app');
var app = new EmberApp({
babel: {
plugins: ['babel-htmlbars-precompile']
}
});
module.exports = app.toTree(); and this allows to write for example a component test like this: import { moduleForComponent, test } from 'ember-qunit';
moduleForComponent('my-component');
test('it renders', function(assert) {
var component = this.subject({
hello: "world",
layout: hbs`huhu {{hello}}`
});
// Renders the component to the page
assert.equal(this.$().text().trim(), "huhu world");
}); @stefanpenner is this something you had in mind with your gist https://gist.github.com/stefanpenner/38a36298d04b29bbce5f? |
@pangratz - Perfect! |
Alright, I've published the inline precompile stuff as EmberCLI addon: ember-cli-htmlbars-inline-precompile. |
Reading threads like this reminds me why I love the Ember community. 😍 |
The outcome of [emberjs#38][38] is undocumented. Raising a deprecation warning for calls to `this.subject` is a good start, but could be improved greatly. [38]: emberjs#38
Would love to see a few robust examples of this in action. I'm trying to do some pretty basic stuff and I'm not sure the best way of approaching the problem. As a simple example: test('submit-button can be disabled', function(assert) {
this.render(hbs `{{submit-button disabled=true}}`);
assert.ok(this.$('button:disabled').length, 'It rendered a disabled submit button');
// Great! This works.
// Now I want to change the disabled property to false and assert that the button
// is no longer disabled. What's the best way of doing that?
}); I'm sure there is an easy answer to this question (just call render again?), but some more real world examples would be useful for those of us trying to migrate our tests over to this style. Also interested in how we pass more complicated data into our components (arrays, objects, etc...). Love the PR - thanks! Update: rerendered the component and it works, but is that a new instance of the component? Or rerender of the original... |
This should do the trick: test('submit-button can be disabled', function(assert) {
this.set('currentDisabledValue', true);
this.render(hbs `{{submit-button disabled=currentDisabledValue}}`);
assert.ok(this.$('button:disabled').length, 'It rendered a disabled submit button');
this.set('currentDisabledValue', false);
assert.equal(this.$('button:disabled').length, 0, 'It switched to disabled');
}); Links:
|
Ahh - makes good sense! Thanks @rwjblue |
The outcome of [emberjs#38][38] is undocumented. Raising a deprecation warning for calls to `this.subject` is a good start, but could be improved greatly. [38]: emberjs#38
The outcome of [emberjs#38][38] is undocumented. Raising a deprecation warning for calls to `this.subject` is a good start, but could be improved greatly. [38]: emberjs#38
This implements template-driven component integration tests. See previous discussion in #25.
TestModuleForComponent
now offers both a unit test mode (which is unchanged from the present behavior) and an integration test mode (which subsumes previous ModuleForIntegration).The default is integration test mode, becauseI am 100% convinced that is the way most people should be testing most components most of the time.We switch to unit test mode if you provide an explicitneeds
argument, or sayunit: true
, or sayintegration:false
.In integration mode, tests are template-driven, giving you full and intuitive access to all features of components, including passing them block templates, yielding block params, and action handling.
People with existing component unit tests that already have aneeds
parameter (which I expect is most of them) will not be affected by this change. But people with component unit tests and noneeds
parameter will get failing tests, until they addunit: true
to their module. This is a very low-cost upgrade pain, so I think it's worth it for achieving a simpler API that nudges people toward the right choice by default.Edited to add: after feedback, I changed this to not be a breaking change. Unit test mode is still the default, but we will hit you with a deprecation warning unless you add an explicit
needs
orunit:true
, which will protect you from a future change that makes integration mode the default.I have tested this change with both ember-mocha and ember-qunit, and no changes are actually required in either of them to use this functionality. We will want to update their ember-cli blueprint generators however, so that they can generate both unit and integration tests for components, and so they place them under an appropriate path (like
tests/integration
instead oftests/unit
).