-
-
Notifications
You must be signed in to change notification settings - Fork 868
Testing guide update for new Ember Qunit API #2145
Testing guide update for new Ember Qunit API #2145
Conversation
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.
Thanks for getting things underway!
- It would be awesome to cross-link to http://api.qunitjs.com/QUnit/module in the section discussing
module
(and make it clear that this is "just" theQUnit.module
function). - It would be great (IMHO) to avoid talking about "unit", "integration", or "acceptance" tests, and instead reference them around what they are testing. The "classic testing types" are actually really confusing to anyone that has had prior experience with the terms (and our versions don't directly relate to their experience).
- In the cases where we can, I'd like if we could avoid using models as the type being tested (obviously in the "model tests" section we can't avoid it). This is because models actually are a pretty bizarre and uncommon object type. Perhaps we could use services instead (e.g. in the
Testing Object Methods
section)?
source/testing/acceptance.md
Outdated
apply to all test cases contained in this module. You can read more about module specific setup and | ||
teardown in the section [Setup and teardown](Acceptance#setup-and-teardown). | ||
Scoping your tests with `module` also allows you to execute your tests independently from other tests. | ||
For example, to only run your tests from your `login` module, run `ember t -m 'Acceptance | login'`. |
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.
It may be better to use the longer form of the various flags/commands in the docs. Its probably much easier to remember ember test --module
than ember t -m
...
source/testing/acceptance.md
Outdated
await visit('/posts/new'); | ||
await fillIn('input.title', 'My new post'); | ||
await click('button.submit'); | ||
assert.equal(this.element.querySelector('ul.posts li:first').textContent, 'My new post'); |
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.
Just begs for qunit-dom
😝 . I'll try to tackle that (or convince @Turbo87 to take a stab) in a future RFC...
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.
Yes, I definitely agree - let's update the examples accordingly as soon as we use qunit-dom
by default as well
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'm curious as to why we don't use find
, findWithAssert
and findAll
a la ember-cli-native-helpers
instead of qunit-dom
. They could have been put right into @ember/test-helpers
. It would be a much smaller change of syntax.
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'm interested in the answer to @Gaurav0 question here as well. @jessica-jordan / @rwjblue do you know if the long-term thinking is to shift away from find
? Or is this just alternate syntax to using find()
? Will one of them be preferable over another with the long-term changes to components? What we teach here is likely to be what folks start using who are new to testing, so would love to point them in the best direction possible ...
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.
find
and findAll
both exist now from @ember/test-helpers
, but I still believe that we should primarily teach this.element.querySelector
. The goal of the recent testing refactors is to reduce “magic” by making it very clear what is going on. find
is yet another possible piece of “magic”, whereas element.querySelector
is “just” normal DOM API’s...
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 think there might be another problem here: :first
is not a valid CSS selector and only compatible with jQuery/Sizzle.
source/testing/acceptance.md
Outdated
}); | ||
``` | ||
|
||
`Await`ing the execution of the asynchronous helpers this way, will ensure that assertions are always made **after** the |
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.
Probably shouldn't capitalize here, though I agree that its a tad odd either way.
source/testing/acceptance.md
Outdated
visit('/posts/new'); | ||
fillIn('input.title', 'My new post'); | ||
click('button.submit'); | ||
andThen(() => assert.equal(this.element.querySelector('ul.posts li:first').textContent, 'My new post')); |
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.
andThen
is completely gone in tests that are setup with setupApplicationTest
(it is only available in "legacy" acceptance tests)
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.
Sweet, I wasn't sure about that yet. I can update / remove that section then to only discuss the async / await
syntax instead
const someThing = this.subject(); | ||
someThing.set('foo', 'baz'); | ||
test('should correctly concat foo', function(assert) { | ||
const someThing = run(() => this.owner.lookup('service:store').createRecord('some-thing')); |
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.
The required run
wrapping makes me sad. Cross linking emberjs/data#5247 with a discussion around whats going on. We have a path forward, just need a champion to tackle fixing it...
`subject` function an object containing the instance variables you would like to initialize. For example, to initialize | ||
the property 'foo' in our object under test, we would call `this.subject({ foo: 'bar' });` | ||
See that first, we are creating a new testing module using Qunit's `module` function. This will scope | ||
all of our tests together in one package that can be configured and run independently from other modules |
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.
s/one package/one group/
?
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.
Yes, I like the wording and can update accordingly 👍
See that first, we are creating a new testing module using Qunit's `module` function. This will scope | ||
all of our tests together in one package that can be configured and run independently from other modules | ||
defined in our test suite. | ||
Also, we have used `setupTest`, one of the several unit-test helpers provided by 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.
Should the various references to other packages (ember-qunit in this case) be links?
8db8a0b
to
5a4682f
Compare
assert.equal(this.$().text().trim(), 'You currently are located in Beijing, China', 'location display should change'); | ||
assert.equal(this.element.textContent.trim(), | ||
'You currently are located in Beijing, China', 'location display should change'); | ||
}); | ||
}); | ||
``` | ||
|
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 wonder if it makes sense to skip the Waiting on Asynchronous Behavior
section completely, as I feel we'd like to encourage the use of async / await
over custom helpers in the Guides. Any thoughts?
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.
Assuming that async/await covers the same functionality as wait
, I say we drop the wait explanation. We may still want to cover something in terms of "handling async." It tripped me up as a beginner.
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.
A major 👍 on dropping the entire wait explanation below, let's do it. It's been a pain 😀
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.
A few small things RE wait.
I agree that this has been a somewhat difficult thing for us to explain historically, but now that things are essentially the same between all test types I’m hoping it becomes a bit easier...
wait()
has been renamed to settled()
in @ember/test-helpers (because await wait()
hurts my eyes).
Generally speaking, I do think it’s important to review the concept of “settled” and to explain that you may need to await settled()
to properly pause execution. Also, FWIW nearly all of the helpers discussed here are internally return settled()
...
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.
The "Testing User Interaction" section above (once updated) will already use async/await
to wait for async effects of user actions so IMHO this section can be dropped
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.
Generally speaking, I do think it’s important to review the concept of “settled” and to explain that you may need to await settled() to properly pause execution. Also, FWIW nearly all of the helpers discussed here are internally return settled()...
I think that's a good point - it might make sense to explain that in this section, too even though the concept of how to use these ready-to-use dom helpers has already been introduced before. I try to update that section accordingly then
5a4682f
to
36df185
Compare
Just checking in here, how are things going? Everything still on track for 3.0 (roughly 4 weeks from now...)? |
36df185
to
77d42db
Compare
@rwjblue My mistake, that I haven't followed up on this one for a while. I'll just be fixing up some of the links and I'll check if I can another review from someone from the learning team as well |
a553f07
to
353f473
Compare
source/testing/index.md
Outdated
There are three different classifications of tests that you will need: | ||
**Acceptance**, **Unit**, and **Integration**. | ||
There are four different types of test setups you can choose from depending on what you will need to test. | ||
Let's have a look at those test types and when each of them is the most useful. | ||
|
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 think it's good to list all 4 types in this intro. I would not describe this as a blocker though, just nice to have, and could be added later.
It may even be worthwhile to mention that "in the past, these were sometimes referred to as Acceptance, Unit, and Integration tests." I could see this change making it hard for people to look back at old materials and figure out what's going on. Edit, just saw Robert's comment to the contrary.
source/testing/index.md
Outdated
including being rendered from a template and receiving Ember's lifecycle hooks. | ||
|
||
Examples of integration tests are: | ||
Examples of these mid-complex tests are: |
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.
Is mid-complex a specific term known in generic testing/new Ember testing API? I found this a little confusing, like it was a different type of test.
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.
No, it isn’t a term related to the changes in testing API
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.
Thanks Robert. Let's avoid that label then.
The only blocker I see on the first pass is the |
353f473
to
5dfbdce
Compare
@jenweber I updated the PR to your comments so far - feel free to also comment on any punctuation / wording suggestions or to add the changes as another commit - whatever is easiest for you |
source/testing/acceptance.md
Outdated
await visit('/posts/new'); | ||
await fillIn('input.title', 'My new post'); | ||
await click('button.submit'); | ||
assert.equal(this.element.querySelector('ul.posts li:first').textContent, 'My new post'); |
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'm curious as to why we don't use find
, findWithAssert
and findAll
a la ember-cli-native-helpers
instead of qunit-dom
. They could have been put right into @ember/test-helpers
. It would be a much smaller change of syntax.
// wrap asynchronous call in run loop | ||
run(() => player.levelUp()); | ||
// wrap asynchronous call in run loop | ||
run(() => player.levelUp()); |
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.
Is it possible to await
this?
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 think the run
loop here is one is good to go without await
ing it
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.
Should we link to some additional info about the runloop here? We have a page at https://guides.emberjs.com/v2.18.0/applications/run-loop/ that might at least help explain it if this is one of the first times folks have heard about it ...
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.
Agreed.
FWIW I’m hopefully that future work will allow us to remove all manual Ember.run wrapping (in both tests and app code) and rely on the existing systems to handle “waiting” for scheduled tasks (e.g. a rerender in xyz ms).
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 added a link to the run loop documentation in a short sentence below that code example
data/pages.yml
Outdated
- title: "Application Tests" | ||
url: "testing-application" | ||
- title: "Testing Basics" | ||
url: "testing-basics" |
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 is important - we need to either a) figure out redirects rather than killing the old links, or b) keep the same urls for now and do the redirect/corrections when guides-app launches. I recommend the 2nd option. URLs are like public API and we should avoid changing them.
source/testing/index.md
Outdated
|
||
There are three different classifications of tests that you will need: | ||
**Acceptance**, **Unit**, and **Integration**. | ||
There are four different types of test setups you can choose from depending on what you will need to test. |
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.
There are four different types of test setups you can choose from:
source/testing/index.md
Outdated
project have not regressed, and that the project's goals are being met. | ||
Some tests will require you to test user interaction and application flow. | ||
In these kind of tests you would like to be able to interact with the application in the same ways that a user would, | ||
by doing things like filling out form fields and clicking buttons. |
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.
In these kind of tests, you interact with the application in the same ways that a user would, such as filling out form fields and clicking buttons.
source/testing/index.md
Outdated
In these kind of tests you would like to be able to interact with the application in the same ways that a user would, | ||
by doing things like filling out form fields and clicking buttons. | ||
This will ensure that the features within a project are basically functional, | ||
and ensure that the core features of a project have not regressed, and that the project's goals are being met. |
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.
Application tests ensure that the interactioons within a project are basically functional, the core features of a project have not regressed, and the project's goals are being met.
source/testing/index.md
Outdated
|
||
Some specific examples of units tests are: | ||
Some tests serve as a middle ground between those tests, which require a full application setup and those, | ||
which interact with specific code algorithms on a micro level. |
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.
What about when you need to test the middle ground between a full application setup and unit tests of specific functions? Consider rendering tests.
source/testing/index.md
Outdated
Some specific examples of units tests are: | ||
Some tests serve as a middle ground between those tests, which require a full application setup and those, | ||
which interact with specific code algorithms on a micro level. | ||
Oftentimes these tests will verify interactions between various parts of the application, |
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.
often
source/testing/index.md
Outdated
|
||
### Simple Unit Tests | ||
|
||
If you would like to test an isolated, small-scoped functionality you can create a unit test. |
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.
comma after functionality
source/testing/index.md
Outdated
### Simple Unit Tests | ||
|
||
If you would like to test an isolated, small-scoped functionality you can create a unit test. | ||
Unit tests are the go-to testing type, if you neither require user interactions nor an application container setup to create your test case. |
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.
no comma
source/testing/index.md
Outdated
### Testing Frameworks | ||
|
||
[QUnit] is the default testing framework for this guide, but others are supported through third-party addons. | ||
|
||
### Testing Blueprints | ||
|
||
By default whenever you are creating a new component, helper, service or any another module in your app, |
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.
comma after service (just an Oxford comma - it's grammatically correct either way)
@jessica-jordan I had another peek and it looks good! I can't review code blocks for accuracy but it looks like you hit all the teeny things I found on the first go. There's one thing that should have a comma taken out, commented above. Thanks again for all your work! |
3c92831
to
b516f8e
Compare
@jenweber Thanks for the quick review, I updated the branch according to your latest comment |
source/testing/acceptance.md
Outdated
Scoping your tests with `module` also allows you to execute your tests independently from other tests. | ||
For example, to only run your tests from your `login` module, run `ember test --module='Acceptance | login'`. | ||
`setupApplicationTest` deals with application setup and teardown. | ||
The last few lines, within the function `test`, contain an example test. |
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.
Both commas on this line (after lines
and then after test
) can be taken out
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'd simplify to "The test
function contains an example test."
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.
@jessica-jordan This is great, thanks so much for all your hard work on it. I've made some more editorial comments here, but let's clean those up and merge this! 🎉
source/testing/acceptance.md
Outdated
await visit('/posts/new'); | ||
await fillIn('input.title', 'My new post'); | ||
await click('button.submit'); | ||
assert.equal(this.element.querySelector('ul.posts li:first').textContent, 'My new post'); |
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'm interested in the answer to @Gaurav0 question here as well. @jessica-jordan / @rwjblue do you know if the long-term thinking is to shift away from find
? Or is this just alternate syntax to using find()
? Will one of them be preferable over another with the long-term changes to components? What we teach here is likely to be what folks start using who are new to testing, so would love to point them in the best direction possible ...
source/testing/acceptance.md
Outdated
await visit('/posts/new'); | ||
await fillIn('input.title', 'My new post'); | ||
await click('button.submit'); | ||
assert.equal(this.element.querySelector('ul.posts li:first').textContent, 'My new post'); |
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.
We could also use this.element.querySelector
here and make it say something akin to:
assert.equal(this.element.querySelector('ul.posts li:first').textContent, 'My new post'); // alternate syntax to find('ul.posts li:first')
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.
:first
is not a valid CSS selector
source/testing/index.md
Outdated
|
||
In the example scenario above, some acceptance tests one might write are: | ||
In the example scenario above, some of these tests you might write are: |
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.
some of the tests
source/testing/index.md
Outdated
|
||
Some specific examples of units tests are: | ||
What about when you need to test the middle ground between a full application setup and unit tests of specific functions? | ||
Consider rendering tests. |
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.
Consider using rendering tests
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 find this sentence difficult since we haven't actually introduced "unit tests" yet.
source/testing/index.md
Outdated
Consider rendering tests. | ||
Rendering tests will verify interactions between various parts of the application, | ||
such as behavior between UI controls. | ||
Typically they require an UI to be rendered to be able to execute user interactions in the testing scenario. |
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.
Typically they require the UI
source/testing/testing-helpers.md
Outdated
}); | ||
``` | ||
|
||
As seen in the [Writing Helpers] guide. The helper function expects the unnamed | ||
arguments as an array as the first argument. It expects the named arguments as | ||
an object as the second argument. | ||
|
||
Now we can move on to an integration test. Integration testing helpers is done | ||
with the `moduleForComponent` helpers, as shown in [Testing Components]. | ||
Now we can move on to a more complex test case, that ensures our helper is rendered correctly as well. This can be done |
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.
test case (and then snip the comma)
// wrap asynchronous call in run loop | ||
run(() => player.levelUp()); | ||
// wrap asynchronous call in run loop | ||
run(() => player.levelUp()); |
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.
Should we link to some additional info about the runloop here? We have a page at https://guides.emberjs.com/v2.18.0/applications/run-loop/ that might at least help explain it if this is one of the first times folks have heard about it ...
source/testing/testing-routes.md
Outdated
The action `displayAlert` contains the code that is called when the action is | ||
received, and the private function `_displayAlert` performs the work. While not | ||
necessarily obvious here because of the small size of the functions, separating | ||
code into smaller chunks (or "concerns"), allows it to be more readily isolated |
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.
drop the comma after (or 'concerns')
source/testing/testing-components.md
Outdated
@@ -133,25 +181,26 @@ We recommend using native DOM events wrapped inside the run loop or the [`ember- | |||
Using jQuery to simulate user click events might lead to unexpected test results as the action can potentially be called twice. | |||
|
|||
```tests/integration/components/magic-title-test.js | |||
import { moduleForComponent, test } from '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.
Just above this section we have an entire paragraph that seems to have been made obsolete (about native DOM events, jQuery events, etc). Can we drop that while we're here?
assert.equal(this.$().text().trim(), 'You currently are located in Beijing, China', 'location display should change'); | ||
assert.equal(this.element.textContent.trim(), | ||
'You currently are located in Beijing, China', 'location display should change'); | ||
}); | ||
}); | ||
``` | ||
|
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.
A major 👍 on dropping the entire wait explanation below, let's do it. It's been a pain 😀
@@ -262,11 +312,11 @@ To stub the location service in your test, create a local stub object that exten | |||
`Ember.Service`, and register the stub as the service your tests need in the |
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.
@Turbo87 Here's also the updated example on how to stub Services - can you take a look if this is good to go?
Not sure if it needs to be in the guide, but you can also import a service, extend it to stub a method or two, and register it in place of the existing service. |
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've added a few comments. Some of them are not actually related to the diff, but to the general structure of the guide which was already in place before Jessica adjusted it to the new APIs.
IMHO we should land this with the necessary fixes soon, but then revise it a little. Specifically I would like to see regular QUnit tests and async/await introduced first and then go to the more complex test setups like container, rendering and application test (in that order).
source/testing/acceptance.md
Outdated
Scoping your tests with `module` also allows you to execute your tests independently from other tests. | ||
For example, to only run your tests from your `login` module, run `ember test --module='Acceptance | login'`. | ||
`setupApplicationTest` deals with application setup and teardown. | ||
The last few lines, within the function `test`, contain an example test. |
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'd simplify to "The test
function contains an example test."
source/testing/acceptance.md
Outdated
Scoping your tests with `module` also allows you to execute your tests independently from other tests. | ||
For example, to only run your tests from your `login` module, run `ember test --module='Acceptance | login'`. | ||
`setupApplicationTest` deals with application setup and teardown. | ||
The last few lines, within the function `test`, contain an example test. | ||
|
||
Almost every test has a pattern of visiting a route, interacting with the page | ||
(using the helpers), and checking for expected changes in the DOM. |
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.
"every [application] test" ?
source/testing/acceptance.md
Outdated
await visit('/posts/new'); | ||
await fillIn('input.title', 'My new post'); | ||
await click('button.submit'); | ||
assert.equal(this.element.querySelector('ul.posts li:first').textContent, 'My new post'); |
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 think there might be another problem here: :first
is not a valid CSS selector and only compatible with jQuery/Sizzle.
|
||
There are three different classifications of tests that you will need: | ||
**Acceptance**, **Unit**, and **Integration**. | ||
There are four different types of test setups you can choose from: |
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 sounds a little like they are exclusive to me, like you would actually have to choose just one and can't use the others 🤔
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.
Also I personally would probably find it easier to understand if we go from simple (unit) to complex (application) instead of introducing the complex test types first. I guess that can be "refactored" later though.
source/testing/index.md
Outdated
project have not regressed, and that the project's goals are being met. | ||
Some tests will require you to test user interaction and application flow. | ||
In these kind of tests, you interact with the application in the same ways that a user would, such as filling out form fields and clicking buttons. | ||
Application tests ensure that the interactions within a project are basically functional, the core features of a project have not regressed, and the project's goals are being met. |
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.
most of this is also true for "rendering tests". the main difference is that "application tests" startup the whole app with routing
source/testing/testing-routes.md
Outdated
Testing routes can be done both via acceptance or unit tests. Acceptance tests | ||
will likely provide better coverage for routes because routes are typically used | ||
to perform transitions and load data, both of which are tested more easily in | ||
Testing routes can be done both via application tests or more container tests. Application tests |
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.
both via application tests or more container tests
why "more"? I'd go for "application or container tests"
route.send('displayAlert', expectedTextBar); | ||
// Now use the routes send method to test the actual action | ||
route.send('displayAlert', expectedTextBar); | ||
}); |
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.
IMHO this should be two separate tests, though I don't really see the necessity here for having two different things anyway 🤔
source/testing/testing-models.md
Outdated
// this.subject aliases the createRecord method on the model | ||
const player = this.subject({ level: 4 }); | ||
test('should increment level when told to', function(assert) { | ||
// this.subject aliases the createRecord method on the model |
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.subject
does not exist anymore
|
||
// wrap asynchronous call in run loop | ||
run(() => player.levelUp()); | ||
// wrap asynchronous call in run loop |
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.
there is no async/await here so calling this async in the comment makes it very confusing
const User = this.store().modelFor('user'); | ||
const relationship = get(User, 'relationshipsByName').get('profile'); | ||
// lookup the relationship on the user model | ||
const relationship = get(User, 'relationshipsByName').get('profile'); |
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.
why is this using get(User, ...)
instead of User.get(...)
?
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.
DS.Model.get
is not a method (User
here is a "class" not an instance).
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.
if there is no get
then why not just User.relationshipsByName.get('profile')
?
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.
Because I think relationshipsByName
is a computed property on the constructor (yaaasss this is super weird see here)? I dunno, this kinda confuses me anyways, why do we actually need to enumerate all the relationships on this model class?
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.
Like, IMHO, testing that this type of wiring works is super in the weeds. Ideally you make a profile instance, then make a user instance, then confirm that user.get('profile') === profile;
or something.
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.
yeah, agreed
33b9da0
to
7e0810a
Compare
@rwjblue @acorncom @Turbo87 I updated the PR according to your comments, updating anything that is essential for getting the Guides up-to-date to the new Ember QUnit testing API - leaving out further suggested improvements that are not directly related to my changes. I think the outstanding issues can and should be tackled in another PR though. Thank you for the thorough review! |
7e0810a
to
dee609c
Compare
This updates the acceptance testing section including its code examples according to the new Ember Qunit Pattern proposed through RFC#268.
This updates the unit testing section including code examples according to the new Ember Qunit Testing Patterns proposed in RFC#232.
This updates the integration testing section according to the new Ember Qunit testing patterns proposed in RFC#232.
This updates the testing section about helpers according to the new Ember Qunit testing patterns proposed in RFC#232.
This updates the controller testing section according to the new Ember Qunit testing patterns proposed in RFC#232.
This updates the route testing section according to the new Ember QUnit testing patterns proposed in RFC#232.
This updates the model testing section according to the new Ember Qunit testing patterns proposed in RFC#232.
This updates the introduction into testing introducing new concepts of the Ember Qunit testing API proposed in RFC#232 and RFC#268.
This renames testing types to avoid an arbitrary distinction between application, integration and unit tests, but instead embrace a naming which reflects which kind of test setup is needed to run them. This introduces the testing type titles - application test - rendering test - container test - unit test into the testing section as presented in @Turbo87 's talk "The Next Generation of Testing", Dec 2017.
This updates the title of the former "Unit Testing Basics" section to "Testing Basics" to avoid talking about "container" tests in an originally "unit" test focused section.
Let's get this going and iterate 😎. Thank you everyone for your hard work! |
This PR is an update to the Ember testing guide, updating the code examples according to the new Ember QUnit API proposals from RFC 232 and RFC 268.
This is also updates the naming for different types of tests.
Relates to the quest issue emberjs/ember.js#15933