Test Co-Location #599

NullVoxPopuli · 2020-03-02T17:52:04Z

text/0000-colocated-tests.md

…rectory to the app directory

void-mAlex · 2020-03-02T18:27:17Z

text/0000-colocated-tests.md

+
+#### 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.


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

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)

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

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

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.

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.

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.

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:

Co-locate acceptance tests as the blueprint currently understands them

Maybe add another blueprint for non-route-focused blueprints that probably get generated in tests/<somewhere>.

Gaurav0 · 2020-03-02T19:20:44Z

Currently, all tests must end with -test.js even if they are in tests directory. Why wouldn't it make sense to continue to use -test.js instead of changing it to .test.js?

This way, we would only have to move the file, not rename it as well.

NullVoxPopuli · 2020-03-02T19:24:45Z

Why wouldn't it make sense to continue to use -test.js instead of changing it to .test.js?

the RFC says this: https://github.com/emberjs/rfcs/pull/599/files#diff-9e4ea543242ddd0a6ef8209ac8a058e8R27

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 bundle.

text/0000-colocated-tests.md

jrjohnson · 2020-03-02T19:39:35Z

text/0000-colocated-tests.md

+
+#### 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.


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

mehulkar · 2020-03-03T00:16:11Z

Seems good for component and unit tests, but the complication with acceptance tests does make it harder to justify. That said, it may be possible to put acceptance tests at app/tests, so it's "co-located" with... the app.

Couple other concerns:

how does it affect the tests/dummy in addons
how does this affect what we call the different test types? e.g. unit vs integration tests for components & helpers. cc Test Directories #575

NullVoxPopuli · 2020-03-03T00:42:31Z

put acceptance tests at app/tests, so it's "co-located" with... the app.

that's an interesting idea! would we just want them in tests, or make the directory stand out somehow? __tests__, -tests, etc?

how does it affect the tests/dummy in addons

ha, ya, I totally forgot about addons.
I think for simplicity, I don't want to mess the dummy app, it's already awkward, and I think we are one ore two RFCs from it being less awkward? would be nice to have actual apps instead of a dummy app, imo.

for addon, non-acceptance tests, I could see those being co-located following all the same rules outlined for the app. (just with addon instead of app as the root. During implementation, I would also need to make sure that tests in the addon aren't bundled with a consuming app

how does this affect what we call the different test types? e.g. unit vs integration tests for components & helpers. cc #575

integration vs unit doesn't matter today, aside from the arbitrary folders that those tests get thrown in to.
good question though -- since those concepts wouldn't matter here, I suppose the test type doesn't matter either.
maybe the generated tests are just... tests.... that happen to use setupTest or setupRenderingTest
like,

module('Component | my-component', ...);
module('Service | my-service', ...);
module('Helper | my-helper', ...);
// etc

mehulkar · 2020-03-03T01:53:22Z

integration vs unit doesn't matter today

The blueprints are different

NullVoxPopuli · 2020-03-03T07:02:43Z

The blueprints are different

I meant location-wise

camerondubas · 2020-03-04T02:41:39Z

How would this work for in-repo engines and add-ons? Currently their tests are co-located in the tests directory of the host app.

I work on a large app with multiple in-repo engines, and being able to have test files co-located with their relevant files would be a huge improvement to the development experience.

Regardless, I really like this RFC. Again, working in a big app makes finding test files quite cumbersome as is.

nightire · 2020-03-04T02:56:59Z

I like the idea of test co-location in general, but the acceptance test is the biggest roadblock in front of this. Most people write acceptance tests in the feature/scenario oriented way(according to the latest survey results), it is not necessary to be bounded with routings. Thus, the acceptance tests deserve a dedicated place and it should be easy to navigate.

I suggest we don't have to be picky too much, just add another folder called app/(application|acceptance|feature)-tests/ to serve this purpose.

As for the end of tests filename, I agree to keep -test.js form in order to help developers migrate easily. But it should be easy to support .test.js at the same time.

In-repo add-ons/engines could be another big issue here, but I don't know too much about it, I prefer to using monorepo structure these days. Though I think we don't have to put acceptance tests in the in-repo add-ons because "in-repo" implicates serving and only serving the host application, so the application should take care of the acceptance tests.

What about unit tests and rendering tests in the in-repo add-ons, does co-locate support it? I'm not sure, but it should be addressed in this RFC.

Another thing is once test co-location is settled, the app/ folder is actually not just app anymore, it contains all types of source code, so it should be renamed as src/ or other else...wow, welcome back, unifications! Anyway, this could be another RFC on another day.

Just my personal opinion, but I would like to see test co-location becomes a success.

jelhan · 2020-03-05T06:48:19Z

integration vs unit doesn't matter today, aside from the arbitrary folders that those tests get thrown in to.
good question though -- since those concepts wouldn't matter here, I suppose the test type doesn't matter either.
maybe the generated tests are just... tests....

Agree that we may not want to make such a big deal anymore of the difference but the RFC must include how to deal with it:

How to migrate tests for a component if both integration and unit tests exist? Can't move both to the same location.
What should happen if user tries to generate an unit test with blueprints for a component that already has an integration test?

The same question apply to template helpers and their tests. Possible strategies would be:

Use a -integration-test.js and -unit-test.js suffix in that cases or
skip the tests on automated migration or/and
treat it as an attempt to replace existing test if user tries to generate an unit test for a component that already has an integration test. Ember CLI would ask if it should be overwritten as today.

NullVoxPopuli · 2020-03-09T15:33:30Z

How would this work for in-repo engines and add-ons? Currently their tests are co-located in the tests directory of the host app.

What about unit tests and rendering tests in the in-repo add-ons, does co-locate support it? I'm not sure, but it should be addressed in this RFC.

Just updated the RFC to answer the questions. In short, tests could be anywhere within the entire repo. Maybe not just scoped to app?.

but the acceptance test is the biggest roadblock in front of this.
I suggest we don't have to be picky too much, just add another folder called app/(application|acceptance|feature)-tests/ to serve this purpose.

The intent here (for me) is to provide the least dramatic changes while still enabling a lot of flexibility.
If there is enough deviciveness about acceptance tests, It'd totally be cool to add a default application test location to the .ember-cli file.

As for the end of tests filename, I agree to keep -test.js form in order to help developers migrate easily. But it should be easy to support .test.js at the same time.

I think this is a technical limitation, and nothing we can do anything about. supporting -test.js in the app directory could be a breaking change as -test files would be omitted from the app's bundle -- which is breaking if someone has a component or route ending in -test. Like, maybe drivers-test for a route or something.

How to migrate tests for a component if both integration and unit tests exist? Can't move both to the same location.

Component unit tests, imo, should be deprecated, as they rely heavily on implementation and slow maintenance, buuuut, the generator will already warn and prompt you when there is a file conflict and the migrator will need to de the same.

that said, my advice, is if you want both component integration and unit tests to be co-located, you could certainly rename things to be be -unit-test.js and -integration-test.js. maybe if the migrator can be a little more opinionated about this and create those files for you if it detects a conflict, but I don't think it's a requirement for this RFC.

What should happen if user tries to generate an unit test with blueprints for a component that already has an integration test?
The same question apply to template helpers and their tests.

the generator will warn in case of a conflict, and the user can decide what to do from there. the automated migrator should probably do the same.

hope this helps! I've added some unresolved questions to the bottom of the RFC, and would like people's feedback for in-repo-addons and engines

srowhani · 2020-03-09T16:12:38Z

I like the proposal - would suit integration and unit tests quite well. Thanks for preparing this

bendemboski · 2020-03-10T18:34:25Z

I think this proposal should address:

What about addons?
Where does tests/test-helper.js go?
Where does tests/index.html go?
Where does tests/helpers go?

I think the answer to (1) is pretty clear (s/app/addon/g), just should be called out in the RFC.

The answers to (2) and (3) could definitely be "it stays where it is," but should be addressed in the RFC.

The answer to (4) could be "it stays where it is" although then we need to talk about importing test helpers since I believe most people do a relative import that never traverses outside the tests/ directory. A relative import would still work in this RFC, but I think it would piss off most tooling because I believe the runtime relative path from app/foo.js to tests/foo.js is actually ./tests/foo.js, not ../tests/foo.js as the directory structure would suggest. Maybe we advise people switch to absolute imports for test helpers since it's probably less confusing.

I would propose another option for (4), which is also what I proposed in #575, which is to move test helpers to /test-support (which mirrors addons), and then recommend people use absolute imports. I suppose we could argue for putting test-helper.js and index.html there as well...I'm less sure of that...

NullVoxPopuli · 2020-03-10T18:53:14Z

@bendemboski re: #1, did you see this section: https://github.com/NullVoxPopuli/rfcs/blob/colocated-tests/text/0000-colocated-tests.md#addons-in-repo-addons-and-engines ?

re: the things that aren't tests -- I don't know if this RFC needs to address them, because this RFC is explicitly about {-,.}test.* files? idk. Just trying to keep scope low, ya know?

bendemboski · 2020-03-10T19:42:18Z

@NullVoxPopuli oh yeah, I read it but I guess then forgot about it. That seems to imply that the addon tests would live in app/ instead of addon/? I assuming that's not what you meant...so maybe it just needs a little clarification?

I guess not addressing (2) and (3) is valid, although I think it would be helpful to call them out as explicitly not addressed by this RFC because I think the RFC strongly suggests the question of what happens to them.

For (4), I suppose you could do the same thing, but unlike test-helper.js and index.html, the location of the test helpers has an impact on what needs to be inside the test files, so I think if we're providing an option to move test files somewhere else, we should have a story/advice/something for how they should import their helpers. Could just be suggesting absolute imports, and then we could discuss moving the helpers directory in a separate RFC, but IMO the RFC is incomplete without saying something about how to import test helpers.

…t ;)

NullVoxPopuli · 2020-03-10T20:00:30Z

That seems to imply that the addon tests would live in app/ instead of addon/

I linked to a paragraph explicitly talking about addon :P

anywho, I've updated everything to use {app,addon} where appropriate, so people don't think I'm talking about just one thing all the time. It's a placeholder. :D

NullVoxPopuli · 2020-03-10T20:02:00Z

Could just be suggesting absolute imports,

I'm a fan of this.

Now that I've had some time to think about your suggestions, I think they'd all be useful under the "how we teach this" section.

bendemboski · 2020-03-10T20:27:23Z

I linked to a paragraph explicitly talking about addon :P

Right, that's what I was referring to when I said I read it but I guess then forgot about it. This is the sentence that was confusing me:

For normal addons, living as their own entire project, the lookup rules for apps will apply.

Now that the rules reference app,addon it's perfectly clear though 😄

gossi

Generally in favor of this. FWIW, this folder structure:

app/components/my-component/
- component.ts
- template.hbs
- styles.css
- story.ts (demo)
- story.gmd (docs)
- unit.test.ts (testing imperative API)
- rendering.test.ts (testing declarative API)
- integration.test.ts (testing integration with eg. data-integration, REST-API/GraphQL...)

text/0000-colocated-tests.md

gossi · 2020-03-10T21:04:48Z

text/0000-colocated-tests.md

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


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

do unit tests for helpers exist?

Why use different files? Why not

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

@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.

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?

@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.

gossi · 2020-03-10T21:08:44Z

text/0000-colocated-tests.md

+
+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.
+
+While tests may live anywhere in the `{app,addon}` directory, the default location for acceptance tests will still be `tests/acceptance/`.


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

NullVoxPopuli · 2020-03-10T22:40:54Z

@gossi

Generally in favor of this. FWIW, this folder structure:

Personally, if there would be multiple test files, I'd make a tests folder per component

maybe,

app/components/my-component/
  component.ts
  template.hbs
  styles.css
  -docs/
    - story.ts
    - story.gmd
  -tests/
    - imperative.test.ts
    - declarative.test.ts
    - integration.test.ts

NullVoxPopuli · 2020-07-31T10:20:28Z

The need for this capability has come up a few times in discord (and at work) over the past week or two ;)

lcmen · 2021-01-21T18:19:34Z

@NullVoxPopuli what's the status of this?

NullVoxPopuli · 2021-01-21T19:04:23Z

I think at this point, it needs to be reviewed by a core team member?

MichalBryxi · 2021-03-02T21:43:44Z

text/0000-colocated-tests.md

+ - `{app,addon}/routes/{route-name}.test.js`
+pods:
+ - `{pod-namespace}/{route-name}/route.js`
+ - `{pod-namespace}/{route-name}/.test.js`


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?

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

jelhan · 2021-03-02T22:10:30Z

text/0000-colocated-tests.md

+ - `{app,addon}/routes/{route-name}.test.js`
+pods:
+ - `{pod-namespace}/{route-name}/route.js`
+ - `{pod-namespace}/{route-name}/.test.js`


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?

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?

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.

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

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.

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.

NullVoxPopuli · 2021-03-02T22:54:26Z

So, here are the remaining todos of this RFC (before I bother the core teams)

Find acceptable naming convention for tests while using pods
Lay out details for how blueprints work
- Controller unit tests
- Acceptance tests now co-located with routes
- new multi-route acceptance test blueprint

Needs more discussion:

Do we also want to change how we name tests? or leave that for a different RFC?
- rendering vs unit vs application as the new names (maybe "feature"? idk)

Please comment if I missed something ;)

wagenet · 2022-07-23T23:09:31Z

@NullVoxPopuli what's the status of this? I'd love to get it moving again.

NullVoxPopuli · 2022-07-23T23:34:35Z

It's been a while, and I need to test this, but I think with some clever packager loader rules (webpack for now) using embroider strictest mode, we might be able to achieve this today. I don't think there is anything for ember core teams to do, as this is now a packager responsibility.

If/when I do try this again, I'll probably uncover something special about the tests tree, and I'll open a more specific issue

NullVoxPopuli added 3 commits March 2, 2020 09:20

first draft

aa46051

I have no idea what to do with 'How we teach this'

98a2b8a

add PR link

f05d807

simi reviewed Mar 2, 2020

View reviewed changes

text/0000-colocated-tests.md Outdated Show resolved Hide resolved

NullVoxPopuli added 2 commits March 2, 2020 10:04

update test location

22ba173

add additional drawback related to relative imports from the tests di…

7b7e040

…rectory to the app directory

void-mAlex reviewed Mar 2, 2020

View reviewed changes

NullVoxPopuli changed the title ~~Stage 0: Test Co-Location~~ Test Co-Location Mar 2, 2020

jrjohnson reviewed Mar 2, 2020

View reviewed changes

add some to summary and motivation, as well as an alternative

7fb3d01

jayjayjpg mentioned this pull request Mar 3, 2020

The Ember Times No. 138 - March 6th 2020 ember-learn/ember-blog#548

Merged

25 tasks

amyrlam mentioned this pull request Mar 7, 2020

The Ember Times No. 139 - March 13th 2020 ember-learn/ember-blog#558

Merged

24 tasks

Add notes about addons, in-repo-addons, and engines

4dfee2c

add a couple more questions

c734e72

replace app with app,addon so skimmers don't feel addons were left ou…

73db2de

…t ;)

gossi reviewed Mar 10, 2020

View reviewed changes

NullVoxPopuli added 2 commits March 10, 2020 14:14

add note about the rest of the tests files

4038e17

added pods examples

fc70d23

amyrlam mentioned this pull request Mar 14, 2020

The Ember Times No. 140 - March 20th 2020 ember-learn/ember-blog#570

Merged

22 tasks

NullVoxPopuli mentioned this pull request Apr 13, 2020

Test Directories #575

Closed

MichalBryxi reviewed Mar 2, 2021

View reviewed changes

jelhan reviewed Mar 2, 2021

View reviewed changes

NullVoxPopuli mentioned this pull request Mar 3, 2021

Colocate route+template+controllers #651

Closed

NullVoxPopuli closed this Jul 23, 2022


		#### 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.


		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.

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

Test Co-Location #599

Test Co-Location #599

Conversation

NullVoxPopuli commented Mar 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mehulkar Mar 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gaurav0 commented Mar 2, 2020 • edited Loading

NullVoxPopuli commented Mar 2, 2020

Choose a reason for hiding this comment

mehulkar commented Mar 3, 2020 • edited Loading

NullVoxPopuli commented Mar 3, 2020

mehulkar commented Mar 3, 2020

NullVoxPopuli commented Mar 3, 2020

camerondubas commented Mar 4, 2020

nightire commented Mar 4, 2020

jelhan commented Mar 5, 2020

NullVoxPopuli commented Mar 9, 2020

srowhani commented Mar 9, 2020

bendemboski commented Mar 10, 2020

NullVoxPopuli commented Mar 10, 2020

bendemboski commented Mar 10, 2020

NullVoxPopuli commented Mar 10, 2020 • edited Loading

NullVoxPopuli commented Mar 10, 2020 • edited Loading

bendemboski commented Mar 10, 2020

gossi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bendemboski Mar 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullVoxPopuli commented Mar 10, 2020 • edited Loading

NullVoxPopuli commented Jul 31, 2020

lcmen commented Jan 21, 2021

NullVoxPopuli commented Jan 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullVoxPopuli commented Mar 2, 2021 • edited Loading

wagenet commented Jul 23, 2022 • edited Loading

NullVoxPopuli commented Jul 23, 2022

NullVoxPopuli commented Mar 2, 2020 •

edited

Loading

mehulkar Mar 3, 2020 •

edited

Loading

Gaurav0 commented Mar 2, 2020 •

edited

Loading

mehulkar commented Mar 3, 2020 •

edited

Loading

NullVoxPopuli commented Mar 10, 2020 •

edited

Loading

NullVoxPopuli commented Mar 10, 2020 •

edited

Loading

bendemboski Mar 10, 2020 •

edited

Loading

NullVoxPopuli commented Mar 10, 2020 •

edited

Loading

NullVoxPopuli commented Mar 2, 2021 •

edited

Loading

wagenet commented Jul 23, 2022 •

edited

Loading