Requiring tests to be in a folder named 'test' is a terrible quality metric

[myndzi]

To an extent I can agree that containing tests in a subfolder is useful as a quality metri, but requiring a name for that subfolder is plain awful. It doesn't speak at all to any kind of quality metric about the code itself, only shoehorns authors who are paying attention into a rigid structure, and authors who aren't paying attention into falsely low scores.

Two alternative suggestions:

1) The intent of this checker seems to be about code organization / keeping things tidy. This can be achieved without any specific structure by simply testing the number of "loose files" in the root directory. This should be done on the product of npm packaging so that npmignore is accounted for, and possibly excluding common files such as grunt, gulp, bower, package.json, etc. This method has the advantage of not penalizing small, self-contained packages that only have one file and one test file, for example.

2) Curate a list of acceptable folder names and be open to adding others to the list, as long as they serve the intended purpose. Some opinion is okay with this method (for example "no, 't' is not a good folder name), but it shouldn't be overbearing.

[konobi]

@othiym23 @isaacs got any input?

[tkissing]

While I agree that the "one true folder name" is too strict, I do think conventions are valuable. If I check out your project from GitHub, I don't want to have to read half an hour just to figure out that you put your tests in files ending in .howitshouldbe in the src/lib/extras/verification folder. As I said in https://github.com/konobi/kwalitee/issues/9 any commonly used folder (test, tests, spec) should be acceptable (but it should be exactly one, no more, no less)

[konobi]

Well... the naming of those folders tend to be convention from other langauges, frameworks, etc. but the name itself doesn't provide any value. So, is there something wrong with saying "instead of 'flibblegoobletestsforme' lets use 'test' since we all understand that things in there will be tests"? I've seen several projects where "spec" isn't tests at all, but contains data that is used to generate APIs and the like. It's also nice to have just one place to be able to run a bunch of checks against (vs maybe a bunch or a wrong directory).

[konobi]

@myndzi is your concern about it being detrimental to current available packages (and therefore their score) or is it more about every author having to choose the same folder? These are two different issues.

[myndzi]

@tkissing: Exactly one is exactly my complaint. I don't have a problem with requiring it in the package root, and I don't have a problem with requiring it to be named sensibly. I don't think that "making it easy on the kwalitee module should have any say in the matter.

@konobi: My concern is both, really.

I don't think stylistic choices have a place in a module like this (what to name a folder). The kind of opinionated choices made in linters are okay in linters because linters affect the user/developer, not the consumers, of the package. This module will affect the user and consumers if it does what you want it to.

And yes, I also have a concern that it will affect existing packages should the "rating" become available in some package-search interface: it does the package authors a disservice by unfairly lowering their rating based on a metric that has no bearing on the quality of their product, and it does package searchers a disservice by same.

Ask yourself the obvious question: if I'm looking at a package that put their tests in a subfolder named 'tests', is it in any way a worse package than one that put their tests in a subfolder named 'test'? I posit that the answer is no, and that no matter which single folder name you choose, there will always be cases where the answer is no -- and that's what makes this a poor choice of metric.

[myndzi]

If you want to get around some of the naming/content problems, you could easily just require an extra field in package.json to indicate the location of the tests directory, by the way.

https://docs.npmjs.com/files/package.json#directories

For whatever reason, this seems to have everything but the test directory in it. It would be easy to extend and you could probably get it included in the official docs / spec too. Then your requirement becomes "must specify the directories.test key" and you can mark them down if it's not something within a specific group if you want to be extra pedantic about it.

[konobi]

@myndzi "it does the package authors a disservice by unfairly lowering their rating based on a metric that has no bearing on the quality of their product"... That's their issue... the point of a "kwalitee" metric, is to ensure that the registry as a whole is more easily consumable and verifiable by users.

The point of some of checks is not that it fits in with everything that anyone what's to do ever... It's to ensure that the package can be easily used and identified by users.

For example, have a look at the addon checker... that's probably something that some authors will take on board, others may not... but it's based on confirmation from the core team.

[myndzi]

A package is not more difficult to be used or identified by users by virtue of whether its test directory is plural or singular. I can keep hammering on that point if you want to keep dancing around it, but at the very least as a lowest common denominator it makes a strong point against a single acceptable directory name.

[konobi]

Now or in the future? If it was declared as the go-to name for the future, would it still be a bad check?

[myndzi]

We cross-posted. Re: "That's their issue" -- no, it really isn't. You're aiming to impose an external, somewhat objective, quality metric on everyone who writes modules. The least you can do is not make the metrics arbitrary and worthless, forcing everyone who doesn't agree with your viewpoint to jump through hoops or be "de-rated". In terms of value from a quality perspective, this metric is about as good as "turned three times widdershins before hitting publish" and about as annoying to any author it affects.

Re: now or future, I mean now. Adding a key to package.json is less onerous than restructuring your project, which can easily involve multiple edits (even if it's only two). As well, it provides the flexibility to allow authors to use their preferred structure (within constraints, if you must have them), while informing kwalitee of the information it may want to perform more detailed checks.

[konobi]

An authors preferred layout does not nessecarily have to map to what is published on NPM.

Fairly sure there's plenty of folks who have gulp style things for doing stuff before publishing.

[myndzi]

If you're suggesting that authors should write extra code/configuration in order to work with their tests in one directory and then change that directory specifically to publish to npm, that's just ... I don't even know what to say.

Most peoples' interactions with test directories won't be through npm to begin with. One could go so far as to say that test files themselves shouldn't necessarily even be published to NPM. And that suggestion multiplies the amount of work you'd be putting on authors.

Can you try answering the simple question I posed before? What makes 'test' better than 'tests' to such a degree that it belongs as a scored validation metric in this module? If you can't at least answer that satisfactorily, the rest of the discussion is meaningless.

[konobi]

"test" is more common place on the registry than any other name for a testing folder.

[myndzi]

That doesn't make it better, just more common.

[konobi]

How does anything other than "test" make it better?

[myndzi]

It doesn't. That's the point. Having one single name is not a useful quality metric, because there are multiple, equally valid names.

[konobi]

Actually, having a single name is a useful metric.

[myndzi]

Well, I guess you told me. If you can't come up with a reason, just assert that it is so? I'm outta here. I only posted this here as a courtesy to you, and to get feedback from other people. If you're going to continue to stubbornly and without any discernible or reasonable cause continue to just go back and forth, it's a waste of my time.

[konobi]

So, having a single place to go to, wouldn't be a valuable metric for other tools? Run tests in the same folder, but maybe this time with coverage information, also maybe with some extra security checks and the like? Note... that it's much more likely for these things to be outside of the realm of "kwalitee".

If you can suggest another point to go for, or provide a PR of tooling to detect which tests finally boil down to a single directory, that'd be awesome. Other wise it's just "test" vs "cat /dev/random".

[myndzi]

Having a single place to go could, conceivably, be a useful data point (not quality metric) for other tools to use. If they could also magically figure out the entry point, configuration, test runner, test running options, and the rest of the project hierarchy for your proposed coverage idea. Or, they could just use npm test, which uses package.json, the designated metadata container for npm, to tell them how to accomplish that goal.

You know, the same way I suggested using it to perform this almost completely irrelevant "quality" check.

[konobi]

Still not hearing why "test" isn't a good default.

[myndzi]

You're not suggesting it as a default. You're suggesting it as the one and only acceptable option.

(To repeat/summarize: It is not a good one and only acceptable option because there are multiple, equally valid, acceptable options, which do not inherently commend or condemn a project solely by virtue of their naming.)

[konobi]

Sure... as a forward looking task. Based on data.

[myndzi]

If by that you mean, you are looking forward to a day when every project on NPM uses test for its tests, that is only commendable if there's a reason for it to be so. Which there isn't. Which leaves this as "I want everybody to do things the way I prefer, because I say so." That's the attitude of a bully.

[konobi]

That's on the assumption that there's ego attached... which I've already shown, has gone out the window when I had to get "kwalitee" to adhere to it's own suggestions. "I want everybody to do things the way I prefer", essentially boils down to an API... If you want to interact and do lots of stuff, you need to adhere to an API.

[myndzi]

Wow, you renamed a directory. In contrast to that, you aim to publish the single source of truth about what makes a project good, and you're putting in a terrible showing for the management of that project. You're making it about you and what you want rather than an open discussion with the community about what makes sense. Not a single reply, here, or on IRC, has had even an inkling of consideration for the concern I raised or the multiple solutions I suggested that could address it. When I suggest why it's a bad idea, you change the goal post and attack something else I said. When I respond to that, you come back and say "Still not seeing any good reasons".

Project structure isn't an API. The API for interacting with a Node project is defined by package.json. In fact, the only useful way to interact with the test directory of a project is through npm. Which, conveniently, allows the user to configure how tests get run. This is done for more reasons than aesthetics or preference. It adds flexibility, and decouples the project structure from how you interact with it. Encouraging tight coupling where none is necessary is the opposite of good practice.

[konobi]

Well, if multiple names are really a concern for you... you're welcome to submit a PR with all the directory names that you can think of. These checkers are really not about me at all... it's all about what's best for the node community going forward over the next year or 10.

[myndzi]

To be honest, the package.json solution is much nicer. Of course, I'm not inclined to do the work of changing your code to support this suggestion given the way this conversation has gone -- since it seems obvious that you will simply reject the PR.

Should you state that your intent is to accept such a PR, I will gladly write the code.

[konobi]

shrug it's up to you, and the rest of the community. You can always ask folks to add their +1 here.

[myndzi]

In other words: no.

[konobi]

BTW... my intention has always been to hand off the maintainership of this module elsewhere.

[konobi]

SO... in terms of the title of this issue, there has been no indication that it's better/worse than any other name, so it will remain as "test" until there is a compelling case otherwise (this may come up as additional checks/checkers are brought on board, but for now, it's the most basic level for now.

[myndzi]

The title of this issue is that requiring a specific name is a bad metric. Not that other names are better. If you don't find "this test doesn't indicate quality" to be a compelling metric, I have dismal hopes for the future of this project.

[konobi]

again... "quality" does not map to "kwalitee". Quality is only gauged by reading the code. However... there's definitely a role for "kwalitee" in terms of those packages that are uploaded to the npm registry. Other than the "test" folder name... there are many other checks there that are very relevant to all users. So, is your issue just a very small section of that vs the overall goal?

[myndzi]

"kwalitee", by my understanding, aims to distill various properties of a published npm package into a number, with a higher number representing a project of higher quality by those metrics; the intent being to give browsers some objective gauge they can use as a basis for evaluating whether to consider using a package.

The description in the readme is vague as to what "metric" means: This package is designed to give a community biased and opinionated metric to packages on NPM.

It is presumed, by the similarity of the name to "quality", and also by the other kinds of tests being made, that this "metric" is intended to represent closeness to "best practices" and "great packaging".

There is, indeed a role for this kind of rating system, and I support its coming into existence. But only if its focus is in the right place, that place being one that rewards packages that adhere to good practices and packaging and punishes packages that do not. (And most importantly: does not punish packages that adhere to good practices!)

The core of my issue is that this metric exists in a poor form, unapologetically, and without apparent intent to, and I quote, "change over time, with new metrics, improved metrics and perhaps even complete reversal on metrics that may be considered bad in future." when confronted with an extremely basic suggestion, multiple possible solutions, and no apparent counterargument other than "this is already the way it is".

So, while this issue was opened for the aforementioned individual metric, it has expanded to become representative of a much deeper problem.

(Aside: I find it unfathomably ironic that I went to clone the project to submit your PR anyway, and the one test that fails is the test that checks for this very issue.)

[isaacs]

Probably almost no packages today actually have a "directories.test" field in their package.json.

The common patterns I've seen are to put one or more files in a test/ directory or to have a single test.js file.

In either case, all that matters is that the scripts.test field in package.json:

1. exists
2. passes
3. for bonus points, provides a coverage report of some kind

Doing all this on a continuous basis by using a .travis.yml (and having reports on coveralls.io also) is even better.

Why should the folder name matter, when it's not any harder to check those other things? If the tests pass, and cover the codebase effectively, that's probably what you'er really looking for.

Putting tests in a specific folder and naming them all *.t makes a lot of sense in Perl land where there is a bit more convention on how test harness tools work. But JS is more of a configuration-over-convention kind of place. The only reliable convention is "run npm test and it should pass".

[konobi]

The name doesn't matter in the long run... just that we know which directory the tests are in... using a "configuration-over-convention" method. That way we can enable other checks, for things like ensuring all the devDependencies are declared, that we can add excludes for coverage tooling.

One thing will be to ensure that tests are using the correct comparators and other lint-esque things to ensure that tests are testing what they're expected to be testing.

Yes... scripts.test, tests passing and coverage is important and there are checks for those... but there are plenty of other checks.

I'm not planning on running this in an automated fashion for reporting somewhere, but make it available as a tool for authors who'd like a bit more guidance on what the best practices are, etc.