Skip to content

Test Co-Location #599

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.

Sign up for GitHub

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

Closed
NullVoxPopuli wants to merge 11 commits into from
Closed

Test Co-Location #599

Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
aa46051
first draft
NullVoxPopuli Mar 2, 2020
98a2b8a
I have no idea what to do with 'How we teach this'
NullVoxPopuli Mar 2, 2020
f05d807
add PR link
NullVoxPopuli Mar 2, 2020
22ba173
update test location
NullVoxPopuli Mar 2, 2020
7b7e040
add additional drawback related to relative imports from the tests di…
NullVoxPopuli Mar 2, 2020
7fb3d01
add some to summary and motivation, as well as an alternative
NullVoxPopuli Mar 2, 2020
4dfee2c
Add notes about addons, in-repo-addons, and engines
NullVoxPopuli Mar 9, 2020
c734e72
add a couple more questions
NullVoxPopuli Mar 9, 2020
73db2de
replace app with app,addon so skimmers don't feel addons were left ou…
NullVoxPopuli Mar 10, 2020
4038e17
add note about the rest of the tests files
NullVoxPopuli Mar 10, 2020
fc70d23
added pods examples
NullVoxPopuli Mar 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
210 changes: 210 additions & 0 deletions text/0000-colocated-tests.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,210 @@
- Start Date: 2020-03-02
- Relevant Team(s): Ember CLI
- RFC PR: [#599](https://github.com/emberjs/rfcs/pull/599)
- Tracking: (leave this empty)

# Co-located Tests

## Summary

As apps grow, and need to be refactored, the ability to have tightly-coupled concerns grouped together enables faster refactors and better overall upkeep as related files are grouped together -- as has been demonstrated by co-location of Component class/template, the Pods layout, and Module Unification. This RFC proposes to co-locate tests with their concerned behavior under test (component, controller, service, etc), so that drag-and-drop refactoring is easier.

## Motivation

Similar to why people choose the Pods layout, or Module Unification (back when it was available in the canary build), and more recently, component class/template co-location, the ability to group tightly coupled things together allows for much quicker implementation, iteration, and maintenance of any unit of work.

In today's project structures,

- specific tooling is required to be able to quickly jump to a related test (acceptance tests excluded). By co-locating tests, we eliminate the need for tooling-specific functionality.
- if you were to mass-migrate a number of files via automation, it is less complex to move and interact with sibling files than it is to try to find the related files in some other top-level directory.
- it is not clear where a test is located, or if it even exists. Tests for components, for example, can live anywhere in `tests/`, and finding them could be non-trivial for teams that are mid-migration to/from pods or have less than strict PR reviews. Co-locating gives a clear and quick message to the developer that a something has a test.

This aligns with some of the philosophy of the [Module Unification](https://github.com/emberjs/rfcs/blob/master/text/0143-module-unification.md) RFC:

> Subtrees should be relocatable. If you move a directory to a new place in the tree, its internal structure should all still work.

Which becomes more apparent when each subtree makes use of sibling / descendant imports.

## Detailed design

Nothing about existing tests needs to change. All tests currently living in the `tests/` directory may remain there and coexist with co-located tests.

Primary changes needed:

When building an ember app, based on if tests are included in the build or not, as they would be for development and test builds, ember-cli must configure broccoli to skip over test files within the `{app,addon}` directory.

Additionally, for the test bundle, the `{app,addon}` directory must be added to search for files that are tests.

Test files could remain using the current hyphenated suffix that they currently use -- example: `my-component-test.js`.
But, because the only name part that could be searched for is `-test` at the end of the filename, there is the possibility that valid components, controllers, routes, etc could legitimately end with `-test`, causing files to incorrectly get removed or added to the app or test bundles.
To counter this conflict, co-located tests must end with `.test`, e.g.: `my-component.test.js`. Because having extra periods in the filename isn't allowed in framework objects, using `.test` should be fairly safe to search for when building the app and test bundles.

More concisely:

For the app/addon bundle,
- previously
- include: `{app,addon}/**/*.{js,hbs}`
- now
- include: `{app,addon}/**/*.{js,hbs}`
- exclude: `{app,addon}/**/*.test.{js,ts}`

For the test bundle,
- previously
- include: `tests/**/*.{js}`
- now
- include: `tests/**/*.{js}`
- include: `{app,addon}/**/*.test.{js}`


Like with component co-location, there will be different preferences with how to group files together. It may be undesirable to have a flat list of 20 routes, and then an additional flat list of 20 tests for those routes all in the same folder.
The `.ember-cli` file provides the option to further nest components' class and template files as `{app,addon}/components/{component-name}/index.{js,hbs}` via the `componentStructure` property (set to `nested`).
This same flexibility should be available for routes, controllers, etc -- but is out of scope for this RFC as other resolver implementations may be in-flight.
Avoiding implementing `componentStructure` for the rest of the `app` directory does not have any impact on test co-location as the resolver is not involved with finding test file locations. For example, it would be valid to have `app/routes/{route-name}.js` and tests located at `app/routes/__tests__/{route-name}.test.js`.

### Addons, in-repo Addons and Engines

For normal addons, living as their own entire project, the lookup rules for apps will apply.
For in-repo addons and engines, there is the possibility that the implementation of this feature would enable things not possible today.
For example, if the "test finder" followed the glob: `**/*(\.test.js|-test.js)`, (excluding node\_modules, etc), that would include tests that are in in-repo addons and engines,
which today, do not support their own tests -- the in-repo addon or engine still wouldn't support their own tests, exactly, as they would run as part of the parent
app's test suite. This might be desired behavior as it would allow the in-repo addon and engine to finally co-locate their tests with the behavior that those units implement.


### Reference: Where do the existing tests get migrated to?
While tests in `{app,addon}/` could live anywhere, these are to be the default locations for the generators to use.

#### Route

previously:
- `{app,addon}/routes/{route-name}.js`
- `tests/unit/{route-name}-test.js`
now:
- `{app,addon}/routes/{route-name}.js`
- `{app,addon}/routes/{route-name}.test.js`
pods:
- `{pod-namespace}/{route-name}/route.js`
- `{pod-namespace}/{route-name}/.test.js`
Copy link

@MichalBryxi MichalBryxi Mar 2, 2021

Choose a reason for hiding this comment

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

This raises a red flag. Won't this collide with controller tests?

❯ ember g route foo --pods
  create app/foo/route.js
  // therefore: app/foo/.test.js
❯ ember g controller foo --pods
  create app/foo/controller.js
  // therefore: app/foo/.test.js

This does not seem right?

Copy link
Contributor Author

@NullVoxPopuli NullVoxPopuli Mar 2, 2021

Choose a reason for hiding this comment

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

I've been under the impression that controller tests are rarely used and there wouldn't be a conflict.

but, the controller blueprint could totally check if there is a name collision and then output `app/foo/.controller.test.js

Copy link
Contributor

@jelhan jelhan Mar 2, 2021

Choose a reason for hiding this comment

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

A file starting with a dot is very likely to cause confusion as it's hidden on some operation systems. I think this should be avoided.

I get the argument that collision with legitim names for non-test code must be avoided. But I think we need to find a better solution for pod file system layout. Maybe route.test.js and controller.test.js?

Copy link
Contributor Author

@NullVoxPopuli NullVoxPopuli Mar 2, 2021

Choose a reason for hiding this comment

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

A file starting with a dot is very likely to cause confusion as it's hidden on some operation systems.

are all the top-level project files hidden? .eslintrc, .ember-cli, etc?

Copy link

@MichalBryxi MichalBryxi Mar 2, 2021

Choose a reason for hiding this comment

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

  • But this would not be a top-level project files?
  • I think mostly those that are hidden are ... configuration files ... for libraries?

I agree that having test file as "hidden" (dot in front of the name) seems wrong.

Copy link
Contributor Author

@NullVoxPopuli NullVoxPopuli Mar 2, 2021

Choose a reason for hiding this comment

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

aight, how about a hyphen (which is more inline with our current naming)

Copy link
Contributor

@jelhan jelhan Mar 3, 2021

Choose a reason for hiding this comment

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

A file starting with a dot is very likely to cause confusion as it's hidden on some operation systems.

are all the top-level project files hidden? .eslintrc, .ember-cli, etc?

Yes. At least for linux systems. They aren't shown in most file explorers nor in output of ls terminal command.

Copy link

@gnclmorais gnclmorais Mar 3, 2021

Choose a reason for hiding this comment

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

I agree with the dot files not looking that great… I associate them with configuration and settings, not with test or any actual code written for an application. The hyphen sounds like a good compromise.


#### Service

previously:
- `{app,addon}/services/{service-name}.js`
- `tests/unit/{service-name}-test.js`
now:
- `{app,addon}/service/{service-name}.js`
- `{app,addon}/service/{service-name}.test.js`

#### Controller

previously:
- `{app,addon}/controllers/{controller-name}.js`
- `tests/unit/{controller-name}-test.js`
now:
- `{app,addon}/controllers/{controller-name}.js`
- `{app,addon}/controllers/{controller-name}.test.js`
pods:
- `{pod-namespace}/{controller-name}/controller.js`
- `{pod-namespace}/{controller-name}/.test.js`

#### Component

previously:
- `{app,addon}/components/{component-name}.js`
- `{app,addon}/components/{component-name}.hbs`
- `tests/integration/components/{component-name}-test.js`
now:
- `app/components/{component-name}.js`
- `app/components/{component-name}.hbs`
- `app/components/{component-name}.test.js`

or using `"componentStructure": "nested"`:
- `{app,addon}/components/{component-name}/index.js`
- `{app,addon}/components/{component-name}/index.hbs`
- `{app,addon}/components/{component-name}/index.test.js`


#### Helpers

previously:
- `{app,addon}/helpers/{helper-name}.js`
- `tests/integration/helpers/{helper-name}-test.js`
now:
- `{app,addon}/helpers/{helper-name}.js`
- `{app,addon}/helpers/{helper-name}.test.js`
Copy link

@gossi gossi Mar 10, 2020

Choose a reason for hiding this comment

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

what if I want unit and rendering test?

  • {app,addon}/helpers/{helper-name}.unit.test.js
  • {app,addon}/helpers/{helper-name}.rendering.test.js

like this? If so, please add as examples

Copy link
Contributor Author

@NullVoxPopuli NullVoxPopuli Mar 10, 2020

Choose a reason for hiding this comment

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

do unit tests for helpers exist?

Copy link
Collaborator

@bendemboski bendemboski Mar 10, 2020

Choose a reason for hiding this comment

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

Why use different files? Why not

module('...', function() {
  module('...', function(hooks) {
    setupTest(hooks);
    // ...
  });
  module('...', function(hooks) {
    setupRenderingTest(hooks);
    // ...
  });
});

Copy link
Collaborator

@bendemboski bendemboski Mar 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@NullVoxPopuli I believe before RFC232, helpers were tested by importing the helper function and testing it as a Javascript function. I don't think that makes sense now that we have rendering tests, but there are probably still legacy helper unit tests out there.

Copy link
Contributor Author

@NullVoxPopuli NullVoxPopuli Mar 10, 2020

Choose a reason for hiding this comment

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

I mean, those legacy tests will still work. because (imo), it may not be the happy path, people could either leave their files in tests/ or manually rename?

Copy link
Collaborator

@bendemboski bendemboski Mar 10, 2020

Choose a reason for hiding this comment

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

@NullVoxPopuli I agree with you, was just trying to answer your direct question. In fact, if mixing unit and rendering tests, I'd either put them in different modules in the same file (see above), or just put them all in a rendering test module -- you can do "unit tests" in a rendering test environment -- just don't call render()!

I think the one case where tests don't mix in the same module is application and rendering tests. As rwjblue likes to point out, setupTest() and setupApplicationTest() do almost the same thing -- both fully boot an application. setupRenderingTest() is the odd one since it sets up a sandboxed rendering environment. And I have on occasion written both application and rendering tests for the same subject, but I'd argue those are pretty rare, and nested modules (or just custom test file naming) gives a clean answer there.

So IMO maaaaaaaybe there's a "how we teach this" point in here, but doesn't seem necessary since anything like this is pretty off the beaten path and also has a fine answer.


#### Utils

previously:
- `{app,addon}/utils/{util-name}.js`
- `tests/unit/{util-name}-test.js`
now:
- `{app,addon}/utils/{util-name}.js`
- `{app,addon}/utils/{util-name}.test.js`

#### Acceptance Tests

Acceptance tests do not have a core affiliation with any other file, so co-locating them _may_ not make sense, depending on what a particular test file is doing.
Copy link
Contributor

@void-mAlex void-mAlex Mar 2, 2020

Choose a reason for hiding this comment

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

in the interest of not leaving a class of tests alone in the tests folder which would really act to minimize their importance, I would propose a default location for acceptance tests under the app/routes/ folder

reasoning behind that is that in those tests you start off by visiting a route then testing the behavior of it as a whole

Copy link
Contributor Author

@NullVoxPopuli NullVoxPopuli Mar 2, 2020

Choose a reason for hiding this comment

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

is this how most acceptance tests are? I thought to leave them in tests/acceptance/ because I haven't seen a clear pattern (not that I've seen a ton of different people's / companies' acceptance tests)

Copy link
Contributor

@void-mAlex void-mAlex Mar 2, 2020

Choose a reason for hiding this comment

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

that is partly why I left this comment
also judging by the guide https://guides.emberjs.com/release/tutorial/part-1/automated-testing/
that's what an acceptance test looks like:
await visit('/') click around assert stuff

Copy link

@jrjohnson jrjohnson Mar 2, 2020

Choose a reason for hiding this comment

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

The fact that acceptance tests won't be co-located makes this proposal significantly weaker. Now there are two places to look for tests.

Copy link
Contributor Author

@NullVoxPopuli NullVoxPopuli Mar 2, 2020

Choose a reason for hiding this comment

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

I started a poll on twitter: https://twitter.com/tommyjr/status/1234563047873708032

my thoughts are -- what would they be co-located with?
you can co-locate them yourself, as all .test.js files in the app folder would be detected, but I have a hard time justifying changing the default for acceptance tests.

Copy link

@mehulkar mehulkar Mar 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

co-locating acceptance tests with routes would not work for us. We have several application level tests where the entry URL doesn't matter, we're just using it as a shell to test some complex interaction / behavior.

Copy link

@MichalBryxi MichalBryxi Mar 2, 2021

Choose a reason for hiding this comment

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

I'm personally on the side of co-locating acceptance test with wherever the route lives. Most of the acceptance tests I've written do have some entry URL. I think it's the "sane default" here. And IMO this does not break other people's workflows. If it does not work for your case, you can still use the old locations.

Will the generator be able to use old locations? With some flag?

Also note that since the crawling code will use **/*.test.js we can use any location we like.

Copy link
Collaborator

@bendemboski bendemboski Mar 2, 2021

Choose a reason for hiding this comment

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

The current acceptance test blueprint is pretty tied to a single route. I suppose you could then navigate to other routes, but then the auto-generated module and filename would not be very clear, etc. Whenever I create an acceptance test that isn't just/primarily testing a single route, I don't use the blueprint.

So I could definitely see adding another blueprint, e.g. ember g route-acceptance-test my-route vs. ember g acceptance-test test-my-whole-app, but there is clearly a need for a single-route-focused blueprint, and I do think it makes sense to co-locate that one.

So I think my opinion is:

  1. Co-locate acceptance tests as the blueprint currently understands them
  2. Maybe add another blueprint for non-route-focused blueprints that probably get generated in tests/<somewhere>.


While tests may live anywhere in the `{app,addon}` directory, the default location for acceptance tests will still be `tests/acceptance/`.
Copy link

@gossi gossi Mar 10, 2020

Choose a reason for hiding this comment

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

Engines do have a weird setup with the dummy app, so that might be the co-location in that case.
Also I'm not a big fan of the dummy app being used for docs (e.g. ec-addon-docs) and is loaded in (part of) tests... can sb free it up from the identity crisis? :D



## How we teach this


> What names and terminology work best for these concepts and why? How is this
idea best presented? As a continuation of existing Ember patterns, or as a
wholly new one?

"Test co-location" makes sense, because it's what's used by components' current organization.

> Would the acceptance of this proposal mean the Ember guides must be
re-organized or altered? Does it change how Ember is taught to new users
at any level?

The guides would need to update any references to unit/integration tests in the `tests` folder to be in the `{app,addon}` folder.


> How should this feature be introduced and taught to existing Ember
users?

This RFC does not deprecate the old test location -- it only suggests adding test locations -- so people can take their time migrating or completely avoid migrating altogether and wait for a tool to automatically move (nearly) everything for them.

It should be noted that this RFC is not proposing anything happen with non-test files in the test folder.
- `tests/helpers/**`
- `tests/index.html`
- `tests/test-setup.js`

During migration, the `tests/helpers/**` files should switch to absolute imports, if they are not already.

For apps: `<app-name>/tests/helpers/**`

For addons, these helpers may be moved to `addon-test-support`, but also `<addon-name>/tests/helpers/**` should be feasible as well.


## Drawbacks

While the community migrates, there will be a number of apps that either haven't migrated, or will be in the middle of migrating for a while (depending on app size).
It would be benificial to provide a CLI migration tool to automatically move files from today's standard locations to the new locations.

When moving test files, it's possible that the import paths will be wrong. For example, if someone had `../../app/utils/my-utils.js` in their test file, the path would be wrong if the file moved to a new location.

## Alternatives

The alternatie is to rely on tooling, and implement more features into the (Unstable) Ember Language Server. This will always have gaps, as there are many editors and IDEs out there, and not everyone uses the same editor.

Prior Art:
- Jest projects -- where co-location happens everywhere
- Module Unification -- every was co-locatable, even tests -- though there were bugs wit the tests showing up in the app bundle.


## Unresolved questions

- Should the root of test resolution be the project root, or app/addon folder?
- if project root,
- this gives ultimate flexibility, and people could literally do whatever they want.
- but we'd need to also filter out directories, like tmp, dist, node\_modules
- if app/addon folders,
- then we'd need a more static list of supported folders for test lookup
- app, addon, lib, not sure what else
- Given that in-repo addons can't have their own tests, can engines?
- Would bundling all tests in an app into a single test suite have undesirable consequences?