Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only exclude helpers directory when inside a test directory #909

Closed
rymohr opened this issue Jun 9, 2016 · 21 comments · Fixed by #2103
Closed

Only exclude helpers directory when inside a test directory #909

rymohr opened this issue Jun 9, 2016 · 21 comments · Fixed by #2103
Labels
bug current functionality does not work as desired scope:globbing

Comments

@rymohr
Copy link

rymohr commented Jun 9, 2016

Any chance the exclude rule for helpers can be relaxed so it only applies to files within test/helpers? I just ran into a couldn't find any files to test error for a test file that definitely existed. Ended up being because it was in a helpers dir nested deep within the package I was working on.

I wasn't even aware ava had a concept of helpers until I ran into this. Where are they documented?

Here's the related PR #357 and issues #355 #750

Edit: added issue #750

@jamestalmage
Copy link
Contributor

jamestalmage commented Jun 10, 2016

I think the current pattern is here to stay. Instead I think we should try to better document the patterns.

I think a structuring-your-project recipe is in order.

Also, I think our no tests found error should link to it.

@jamestalmage jamestalmage changed the title relax exclude rule for helpers Improve documentation for default file structures Jun 10, 2016
@rymohr
Copy link
Author

rymohr commented Jun 10, 2016

Sounds like you've made up your mind. But for what it's worth, it's not so much better docs on how to structure your project. For me it was the idea of bringing ava in to unit test small chunks of a very large existing project with an established file structure. I like my tests to live side-by-side with the files they are testing and the current helper exclude policy forced me to restructure my project in order to use ava.

Also, by renaming the issue you've made it hard for anyone in the same boat to find this issue in the future. Renaming issues for clarity is fine, but please don't use renaming to change the intent of the issue.

@jamestalmage
Copy link
Contributor

I wasn't even aware ava had a concept of helpers until I ran into this. Where are they documented?

That's what made me assume it was a documentation issue.

I guess it would be possible to provide yet another glob pattern for users to define what constitutes a helper, but we're pretty against adding config options unless there is a strong need for it.

Can you fully define your directory structure?

@rymohr
Copy link
Author

rymohr commented Jun 10, 2016

If it doesn't make sense to relax the exclude rule from helpers to test/helpers then it would definitely be nice to be able to override the ignore.

Here's an example of where we ran into this problem:

client/
  modules/
    surveys/
      helpers/
        prepareScreen.js
        prepareScreen.test.js

In our case we just renamed helpers to utils to get around the issue for now but I still think it's strange that ava chokes on this by default. The node_modules exclude is a smart default but I feel like the helpers exclude is a special case that should be opt-in.

@jamestalmage
Copy link
Contributor

jamestalmage commented Jun 10, 2016

I'm wondering if we couldn't relax it just a bit, to something like this:

helpers/          <- not excluded? - not sure on this one      

src/
  helpers/        <- not excluded
  foo/
    helpers/      <- not excluded
  test-helpers/   <- excluded
  __tests__
    helpers/      <- excluded
    foo/
      helpers/    <- excluded

test/
  helpers/        <- excluded
  foo/
    helpers/      <- excluded

So basically, the helpers directory is not excluded unless it is already in a test specific directory: (./test/, and **/__tests__/ by default).

This gets complicated though, what if you have renamed your test directory in the options:

"ava": {
  "files": [
    "foo/"
  ]
}

Now foo/helpers probably should be considered a helper directory and excluded.

This is all doable, I'm just wondering if it's worth it.

@jamestalmage
Copy link
Contributor

Also, this would be a breaking change for people with a helpers directory in the root of their project (first line of my example above).

@rymohr
Copy link
Author

rymohr commented Jun 11, 2016

Hard for me to offer any input on whether it's worth it or not since I still don't understand what "helpers" are within ava. I prefer to name my tests with an explicit extension so I don't have to worry about where they are within the project (which also avoids the need for the helper exclude).

@jamestalmage
Copy link
Contributor

jamestalmage commented Jun 11, 2016

Early on, we included every file in ./test/**, and **/__tests__/**. The former pattern allows you to create a test directory that mirrors your src directory (this is what most of the maintainers prefer), the latter allows multiple __tests__ directories within your src directory, keeping tests relatively near your sources. Occasionally, you want to share some common code amongst your tests (some test macro / validator / assertion / etc), and it would be pretty inconvenient if you weren't allowed to put them near your tests, so a _ prefix, or a helpers directory allows you to share that common code, even though it's in a directory where everything else is considered a test.

@jamestalmage jamestalmage changed the title Improve documentation for default file structures Rethinking helpers directory exclusion Jun 11, 2016
@jamestalmage
Copy link
Contributor

So, to be clear, I'm suggesting we relax the exclude, but not as far as you initially proposed.

Rather than just ./test/helpers, it would also exclude ./test/foo/helpers, and ./src/foo/__tests__/helpers. However, ./src/foo/helpers would not be excluded.

@jamestalmage
Copy link
Contributor

While we are at it, should we also relax the _ prefix, if the suffix is .test.js or -test.js?

@novemberborn
Copy link
Member

Makes sense to me to only exclude helpers when they're inside a test directory.

Less keen on relaxing the _ prefix rule. It's easier to understand that _-prefixed files are ignored without exception.

@novemberborn novemberborn changed the title Rethinking helpers directory exclusion Only exclude helpers directory when inside a test directory Jun 12, 2016
@novemberborn novemberborn added bug current functionality does not work as desired help wanted and removed question labels Jun 12, 2016
@rymohr
Copy link
Author

rymohr commented Jun 13, 2016

@jamestalmage sounds like a good plan.

@novemberborn can you explain the original reasoning behind the _ prefix rule? I don't see the obvious connection between this file begins with an underscore and this is not a test file.

@rymohr
Copy link
Author

rymohr commented Jun 13, 2016

@novemberborn never mind! Just saw James' earlier comment:

so a _ prefix, or a helpers directory allows you to share that common code, even though it's in a directory where everything else is considered a test

@dak
Copy link

dak commented Jun 24, 2016

I just ran into this as well and spent about an hour trying to debug why tests weren't being found until I stumbled upon the little note in the README about files in helpers directories being excluded.

As @jamestalmage noted, it's preferable to have a test directory structure that mirrors your src directory structure. The problem with the proposed solution is that it's not uncommon to have a helpers directory in a project's directory structure. So ./test/foo/helpers being excluded still precludes properly matching the src directory structure.

Rather than using a name that easily conflicts with common directory structures, why not pick something that will clearly stand out as being an ava helper and also be very unlikely to conflict?

Proposal: Always exclude ava_helpers, and never exclude helpers.

For bonus points, it might be nice to just make this whole thing configurable under an excludes option, so people have control over their own naming conventions and directory layouts.

@jamestalmage
Copy link
Contributor

So ./test/foo/helpers being excluded still precludes properly matching the src directory structure.

That's a really niche scenario, only a portion of our users use ./test/**. A much smaller portion have a helpers directory that they want to mirror inside. The cross section has got to be small. Not sure it is worth it.

We already keep a dependency list of what each test requires during it's run (so our smart watcher knows what to run when tests change). What if during our initial globbing, we scan for helper directories. If none of your tests ever require anything from within the helper directory, we issue a warning that explains helper directories to you. Requiring anything from that helper directory from a test outside the helper directory signals you understand the point of helper directories, and that we don't need to warn you.

@dak
Copy link

dak commented Jun 24, 2016

I'm a little perplexed. Earlier in this conversation you stated:

Early on, we included every file in ./test/**, and /tests/. The former pattern allows you to create a test directory that mirrors your src directory (this is what most of the maintainers prefer)
That seems to me to suggest that the ./test/** pattern is the most common.

The helpers directory is also in common usage, as can be seen by Ava's own use of it.

But even if the percentage of users that use the ./test/** directory structure and have a helpers directory is relatively small, why would you be content to inconvenience a still not insignificant percentage of your users (clearly there's some, as they've commented in this and other issues) over a legitimate issue when multiple options exist to fix it that do not require significant work or refactoring of ava, and, in the case of allowing excludes to be customized, would not have any impact on current installs?

It does appear that there has existed an intent to make excludes customizable as well.

@jamestalmage
Copy link
Contributor

Because being able to add a helpers directory without touching config is super convenient.

In my mind, the fact that your test directory can't be a perfect mirror of your src directory is not a problem. That we surprised you with a behavior that wasn't obvious without a careful reading of the documentation is a big one though. If we detect your misuse and report it to you, we minimize that surprise, without sacrificing convenience.

Adding config is always an option if no other solutions can be found, but it is one we avoid until it becomes absolutely necessary.

@dak
Copy link

dak commented Jun 24, 2016

I feel like we're talking past each other. Why can't we keep the "super convenient" helpers directory default that you prefer but allow others who prefer different defaults to change it?

I'm fine with tools being opinionated when it has a point, but there doesn't seem to be a benefit from being opinionated about the names of directories to exclude. We're talking about a fairly straightforward option to overwrite the default excludes.

I'll even happily try to submit the PR myself.

@novemberborn
Copy link
Member

I'm not opposed to having a way to prevent helpers from being an excluded directory, though that's outside the scope of this issue.

@novemberborn
Copy link
Member

@dak see #736 for improvements to the pattern management.

@rubeniskov
Copy link

rubeniskov commented Apr 1, 2019

Two years later... 😞 any update on this? I think that exclusion makes more injuries than benefits, I have the same "issue" using helpers folder as utilities for a module. It's quite hardcore to change my whole project structure because the test runner can't accept an override exclusion. I think this is very restrictive, and also It tooks me a while to realize of this inconvenient, even when the documentation explicit those requirements.

I asume ava has to be aseptic an unopionated utility. So I'm not totally aggree with this decission. its setup has to be as much simple as it can be. but this config I think do not contribute to much for that motivation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug current functionality does not work as desired scope:globbing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants