Test Directories

[mehulkar]

rendered

[kellyselden]

Other than wondering if tests/rendering or tests/components is better, I think we should do this.

[jgwhite]

I think tests/components might be a better match for the “Testing Components” section of the guides. Also, typically these tests test not just rendering but also triggering events on components.

[jherdman]

To me tests/container isn't any more obvious than tests/unit. I'm otherwise very much in favour of this.

[jrjohnson]

I like test/container because unit is such an overloaded term. So many TDD and test guides talk about how essential unit tests are that I've seen it give Ember developers the impression that they should be writing most of their tests in tests/unit, which isn't true.

[mehulkar]

For the record I don’t have a strong preference on the names other than being consistent. I would lean towards making them consistent before changing them again though, but also fine with a revisit. I’m guessing someone will need to make an executive decision at some point though.

[gossi]

I'm happy with those changes, we discussed it last year and calling a rendering test an integration tests caused confusion (especially since docs and generators are out of sync).

Basically, FWIW:

components/my-component/
- component.ts
- template.hbs
- story.js // demo
- story.mdx // docs
- rendering-test.js // test declarative API
- unit-test.js // test imperative API
- integration-test.js // tests with backend/REST API integration

Using pods here to demonstrate, this is how I'd see the test landscape for components, use the test-files required for your component.

[luxzeitlos]

I think tests/components might be a better match

Rendering tests can also be helpers.

[jelhan]

These list does not include all directories, in which tests are created by existing blueprints. All folders used by Ember Data to place tests in are missing:

• tests/unit/adapters
• tests/unit/models
• tests/unit/serializers
• tests/unit/transforms

Also the tests created for components and helpers if --unit flag is parsed are missing:

• tests/unit/components
• tests/unit/helpers

Same it true for not that popular features like mixins and (instance) initializers.

The RFC should also say what should happen to these. Not sure how the RFC should handle folders in tests that are used by addons not being part of the default blueprint. I guess they should not be touched but this should be stated explicitly in my opinion.

[mehulkar]

I wasn't attempting to solve for Ember Data unit tests! Those generators can keep doing whatever they want IMO. @runspired or someone else from ED, do you want to expand this RFC to address those as well?

[rwjblue]

Overall, I'm very much in favor of this direction. However, I strongly dislike "container" as a name here (I know that you didn't make up this term, and are just using what is currently commonly used). The "container" is (and always has been) a completely private API. Adding a directory structure that mentions a private thing seems very bizarre.

I regret a bit the names we landed on in #268, and perhaps this is good time to discuss those names. In the new testing system, all tests have an application (in fact if you look at the setup difference between setupApplicationTest and setupTest you will see that there is basically nothing done in setupApplicationTest see here).

I think what we should have done was:

• setupApplicationTest will continue to be used in routing related tests (those that use visit)
• setupTest -> setupApplicationTest - as mentioned above, setupApplicationTest actually doesn't do anything more than setupTest anyways, and it is perfectly valid to use setupTest + visit in the same test.
• setupRenderingTest - no changes.

Like I mentioned above, all of them use a full application and it would behoove us to keep that in mind.

[Gaurav0]

The "container" is (and always has been) a completely private API

No it hasn't. Do I need to bring this up again?
https://api.emberjs.com/ember/1.0/classes/Container

[mehulkar]

@rwjblue I'm fine with a totally new structure, but that increases the scope of this RFC to update all the docs again as well.

[rwjblue]

No it hasn't.

Agree to disagree 😸

Do I need to bring this up again?

Only if you want to be wrong again 😜... Show me where in that file the word "public" occurs?

[bendemboski]

I am a big fan of this sort of cleanup. However, I would advocate for making our primary organization structure for the tests/ directory the what rather than the how. By what I mean, what I am I testing, a component, controller, route, modifier, utility, etc.? By how I mean how am I testing it -- in the standard application environment, in a sandboxed rendering environment, or in a pure browser environment (where I don't call any of the setup*Test() methods, meaning I'm probably just testing a utility function)?

In the pre-RFC268 world, the what and the how were tightly coupled via the moduleFor() infrastructure. One of the big and awesome advancements of RFC232/RFC268 is the decoupling of the two -- I can test a service in a rendering test to make sure the properties it exposes are @tracked properly, or I can test it after visit()ing a route if I want to verify some details of it in a specific in-application scenario.

So since they are decoupled, I don't think we should keep them coupled in the directory structure -- I don't think we should be deciding that component tests are rendering tests or service tests are unit or container tests or whatever. Sure, the blueprints will give you a default setup*() method, but the user can easily change that, or using nested modules use multiple setup*() methods in the same file because they want to test the same what using multiple hows.

So here's my proposal -- get rid of the intermediate integration and unit directories and don't replace them. Rather than

tests/
  - rendering
    - components
      - component1-test.js
      - component2-test.js
    - modifiers
      - modifier1-test.js
      - modifier2-test.js
  - containerOrWhatever
    - controllers
      - controller1-test.js
      - controller2-test.js
    - routes
      - route1-test.js
      - route2-test.js
  - unit
    - util1-test.js
    - util2-test.js
  - etc

let's do

tests/
  - components
    - component1-test.js
    - component2-test.js
  - controllers
    - controller1-test.js
    - controller2-test.js
  - modifiers
    - modifier1-test.js
    - modifier2-test.js
  - routes
    - route1-test.js
    - route2-test.js
  - unit
    - unit1-test.js
    - unit2-test.js
  - etc

The tests/ folder will be structured exactly like the app/ and addon/ folders so tests will be easy to find, and the users will have the flexibility to choose the how of their tests and change/vary/combine hows as needed without worrying that suddenly the test file is in the wrong folder or having to split tests for a single service or controller or something across multiple files because they are using two different hows to test the same what in two different ways.

I think this proposal leaves two questions unresolved:

1. The acceptance/ directory
2. The helpers/ directory (since it clashes with the helpers/ directory that would contain tests for template helpers)

For (1), I think I'd lean towards leaving it at tests/acceptance. I think when all the other folders are names of the type of entity in app/ that they are testing, the name acceptance is actually quite descriptive and clear. But there could definitely be a better name that I am just unable to think of...

(2) is a kinda disappointing sticking point, right when I thought my idea was all slick and clean 😥 My two ideas are to name it something like tests/test-helpers or to move it out of the tests/ directory and make it a top-level/sibling like perhaps test-support, which has the benefit of making it match the corresponding directory in addons.

[gabrielcsapo]

I am very in favor of this change. I am going to post in my issue I opened up having the same thoughts.

ember-cli/ember-cli#9065

Reading through the ember guides https://guides.emberjs.com/release/testing/testing-components/ the use of new language like rendering and application tests are greatly appreciated when it comes to onboarding new engineers.

In the current state of trying to connect that language to the supported flags and folder hierarchy it would be greatly helpful to migrate the following words across the board.

acceptance -> application
integration -> rendering
unit -> unit

This might seem pedantic but the language we use to describe the types of tests we are writing is extremely helpful when it comes to how people write their tests.

Given the following example:

tests/integration/components/pretty-color-test.js

import { module, test } from 'qunit';
import { setupRenderingTest } from 'ember-qunit';
import { render } from '@ember/test-helpers';
import { hbs } from 'ember-cli-htmlbars';

module('Integration | Component | pretty-color', function(hooks) {
  setupRenderingTest(hooks);

  test('should change colors', async function(assert) {
    assert.expect(1);

    // set the outer context to red
    this.set('colorValue', 'red');

    await render(hbs`<PrettyColor @name={{this.colorValue}} />`);

    assert.equal(this.element.querySelector('div').getAttribute('style'), 'color: red', 'starts as red');
  });
});

It not obvious to engineers when they onboard that the word integration means your tests are testing the integration between your component and inputs of that component. If this would be flipped to be rendering;

tests/rendering/components/pretty-color-test.js

import { module, test } from 'qunit';
import { setupRenderingTest } from 'ember-qunit';
import { render } from '@ember/test-helpers';
import { hbs } from 'ember-cli-htmlbars';

module('Rendering | Component | pretty-color', function(hooks) {
  setupRenderingTest(hooks);

  test('should change colors', async function(assert) {
    assert.expect(1);

    // set the outer context to red
    this.set('colorValue', 'red');

    await render(hbs`<PrettyColor @name={{this.colorValue}} />`);

    assert.equal(this.element.querySelector('div').getAttribute('style'), 'color: red', 'starts as red');
  });
});

It is extremely pointed that your component is being tested in the context of the inputs it is given and how it is rendered. This similarly could be applied to acceptance as the word acceptance in the context of tests is ambiguous. Everything is considered an acceptance test as you would want them to pass in order for changes to be accepted. If this was changed to be application tests it would be focused on how your application is tested in the context of outside factors like, but not only; api changes, logged in users, errors, etc.

[gabrielcsapo]

Do we have a reason these are called container now? I think unit is generally a good way to look at testing this at the interface level.

[mehulkar]

Not sure where that comes from. I scanned #268 and #119 but they don't seem to be the place where the terminology is introduced. Maybe @rwjblue knows?

[mehulkar]

From his comments above, it does seem like #268 is where they came up:

However, I strongly dislike "container" as a name here

I regret a bit the names we landed on in #268, and perhaps this is good time to discuss those names.

[gabrielcsapo]

Looking through #268 I can't seem to find the introduction of it in the doc or comments.

[Turbo87]

FWIW I fully agree with @bendemboski, that the paths should reflect what is being tested, not how it is being tested.

It is absolutely valid to have a test file for something and use e.g. setupTest() in one test module and setupRenderingTest() in another module in the same file, so making the path reflect what kind of test setup helper was used would seem bad to me.

I also agree with @rwjblue that we should probably merge the various setupFoo() helpers into one. In an ideal scenario we would only use setupTest(hooks); and then using await visit() or await render() determines what kind of thing is drawn in the QUnit testing container.

The suggestion above would also make it massively easier to wrap our test setup helpers with custom setup code, since only a single helper would need to be wrapped, instead of three separate ones.

[NullVoxPopuli]

Combining all the setup* methods would massively improve the story of #599 as well

[mehulkar]

Thanks all for the comments. There are a lot of good suggestions here and my reading is that this RFC as proposed is not worth the change on its own and that the whole testing setup should be revisited. If that's the consensus, can we move this to FCP to close?

[NullVoxPopuli]

so what's the outcome here?
we need another RFC kinda re-thinking our test setup as a whole?

• ideas in this RFC
• combine setup* methods
• maybe also Test Co-Location #599
• fixing the disambiguation of Ember.testing / environment === 'testing'?

[locks]

Moving to FCP to close as requested by the author! :)

[mehulkar]

The main reason I asked for this to FCP to Close is because I don’t know how to move forward and don’t want it to remain indefinitely open. If anyone has thoughts on a way forward with the ideas as proposed, I’m happy to set aside some time, edit the specifics, and add coauthors.