Add 'one-suite-per-file' rule (#103) #105

alecxe · 2016-09-28T17:43:11Z

This is for the issue #103. Please review.

I think I have not described the motivation for this rule well enough. Please see if you can think of a better way or more reasons to have this rule enabled. Thanks!

lo1tuma

Thanks for your PR. I’ve found a few minor issues, please take a look at it.

lo1tuma · 2016-09-29T09:29:24Z

docs/rules/README.md

@@ -15,3 +15,4 @@
 * [no-hooks-for-single-case](no-hooks-for-single-case.md) - disallow hooks for a single test or test suite
 * [no-top-level-hooks](no-top-level-hooks.md) - disallow top-level hooks
 * [no-identical-title](no-identical-title.md) - disallow identical titles
+* [one-suite-per-file](one-suite-per-file.md) - disallow multiple top-level suites in a single file


I’m not sure about the name. I think it is really important that the rule only applies to top-level suites so maybe this should be reflected in the rule name. So maybe one-top-level-suite or max-top-level-suites which could be configured how many top level suites are allowed.
What do you think?

@lo1tuma absolutely, I like the max-top-level-suites - will rename, thanks.

lo1tuma · 2016-09-29T09:42:07Z

docs/rules/one-suite-per-file.md

+This rule enforces having a single top-level suite in a file.
+
+Multiple `describe` blocks is often a sign that a test should be broken down to multiple files.
+One of the possible problems if having multiple suites is that, if you are, for example, going through tests one by one and focusing them (using `fdescribe`), you might not notice `describe` block(s) that are down there at the bottom of a file which may lead to tests being unintentionally skipped.


Is fdescribe an official alias for exclusive test suites? I can’t find it in the mocha documentation.
I guess describe.only is much more common.

@lo1tuma my bad, too much work with jasmine these days. Will fix.

lo1tuma · 2016-09-29T10:16:33Z

docs/rules/one-suite-per-file.md

+
+```js
+rules: {
+   "mocha/one-suite-per-file": [["describe", "context", "suite", "mysuitename"]]


The severity is missing here, it should be: ["error", ["describe", "context", "suite", "mysuitename"]].

@lo1tuma good finding, thanks.

lo1tuma · 2016-09-29T10:20:45Z

lib/rules/one-suite-per-file.js

+    return {
+        CallExpression: function (node) {
+            var callee = node.callee;
+            if (callee && callee.name && suiteNames.indexOf(callee.name) > -1) {


Should we also support modifiers as e.g. descirbe.only, describe.skip etc.?

You could also reuse the isDescribe function.

@lo1tuma yeah, I think we should support all possible suite variations. Will reuse isDescribe, thanks.

@lo1tuma do you think I can/should safely extend the describeAliases array and include describe.only, describe.skip, suite, suite.only, suite.skip, context.only, context.skip?..

I hope so 😄 . I would rely on our tests here, so if no other test fails after changing describeAliases I would say it is safe to change it.

That’s being said, isDescribe currently doesn’t support MemberExpressions like describe.skip, but it could be easily added, just have a look at how it is done in isTestCase

alecxe · 2016-09-29T23:19:46Z

@lo1tuma updated the PR, please review.

lo1tuma · 2016-09-30T09:07:40Z

lib/rules/max-top-level-suites.js

+        'Program:exit': function () {
+            if (topLevelDescribes.length > suiteLimit) {
+                context.report({
+                    node: topLevelDescribes[1],


I guess topLevelDescribes[1] needs to be changed to topLevelDescribes[suiteLimit], otherwise the rule would always report the second top-level describe node, right?

@lo1tuma you are right, fixed.

jfmengels

I would make the option an object with a limit property. Having an object as the options makes it way easier to add new options later, and also clearer when reading the ESLint configuration file.

jfmengels · 2016-09-30T11:48:08Z

lib/rules/max-top-level-suites.js


 module.exports = function (context) {
    var stack = [],
        topLevelDescribes = [],
-        suiteNames = context.options[0] ? context.options[0] : defaultSuiteNames;
+        suiteLimit = context.options[0] ? context.options[0] : defaultSuiteLimit;


You could do context.options[0] || defaultSuiteLimit (shorter) or context.options[0] !== undefined ? context.options[0] : defaultSuiteLimit (maybe a bit more correct, if the user wants 0 suites for some reason)

@jfmengels good point. I was thinking about the case with limit = 0, but decided that would not make sense, though, I agree, this is not the reason to not let it work properly. Thanks!

alecxe · 2016-09-30T15:41:55Z

@lo1tuma okay, please see if it is good enough now.

jfmengels · 2016-09-30T15:44:54Z

I still think the options should be an object (#105 (review))

alecxe · 2016-09-30T15:45:43Z

@jfmengels ah, alright, let me fix that, thanks.

alecxe · 2016-09-30T15:53:15Z

@jfmengels okay, done, please see if this is what you've meant. I like the idea of having an object - explicit and readable. Thanks.

jfmengels

@alecxe This is what I meant 👍

I would be nice to also have a section in the docs that describe the available options. Here is an example.

Related to my other comment for this review, it would be nice to have both valid and failing tests with the options set as an empty object.

jfmengels · 2016-09-30T16:05:32Z

lib/rules/max-top-level-suites.js

+    if (R.isNil(suiteLimit)) {
+        suiteLimit = defaultSuiteLimit;
+    } else {
+        suiteLimit = suiteLimit.limit;


It's possible that users supplied an empty object (or that in the future another option will be available and that the user sets only the value of that one).

The way I usually do this is this way:

var options = context.options[0] || {}; var suiteLimit = options.limit || defaultSuiteLimit;

@jfmengels okay, good idea, added an empty object handling with a corresponding test.

By the way, when I write it as:

suiteLimit = defaultSuiteLimit ? R.isNil(options) : options.limit

I'm getting a lot of TypeError: Cannot read property 'loc' of undefined which I think is a bug in ESLint - so, have to use the explicit if/else in this case.

As far as documenting "options" - I think in the link you've posted "settings" are actually described..but in this case limit is a rule option..

Thanks.

TypeError: Cannot read property 'loc' of undefined

No, that sounds like a normal error.

suiteLimit = defaultSuiteLimit ? R.isNil(options) : options.limit
defaultSuiteLimit is a truthy value, so suiteLimit will evaluate to R.isNil(options), meaning true or false.

When you then compare the number of top-level suites to this boolean, it will evaluate to true if there is at least one. Then, you report an error, but pass undefined as the node (as topLevelDescribes[true] doesn't exist), which ESLint then uses to find loc. Crash!

tl;dr: You should have written it as suiteLimit = R.isNil(options.limit) ? defaultSuiteLimit : options.limit). I would have done it the way I proposed, but your way should work fine too ;)

jfmengels · 2016-09-30T22:05:00Z

LGTM, thanks @alecxe! :)

lo1tuma

LGTM, thanks @alecxe and thanks @jfmengels for the thoroughly review 👍.

alecxe · 2016-10-03T13:01:00Z

Okay, sorry for the rookie mistakes and thanks for being patient!

jfmengels · 2016-10-03T13:05:15Z

No problem (for me at least)! Thanks for your work! :)

Add 'one-suite-per-file' rule (lo1tuma#103)

2552b75

lo1tuma requested changes Sep 29, 2016

View reviewed changes

Renamed the rule, fixed multiple issued found during the review

1442cfd

lo1tuma requested changes Sep 30, 2016

View reviewed changes

jfmengels reviewed Sep 30, 2016

View reviewed changes

alecxe added 2 commits September 30, 2016 11:25

Reporting appropriate suite

4b88dd2

Handling 0 limit

c5ed5d4

Making suite limit configurable as an object

0efd731

jfmengels reviewed Sep 30, 2016

View reviewed changes

Handling empty objects

4a8a686

lo1tuma added the feature label Oct 3, 2016

lo1tuma approved these changes Oct 3, 2016

View reviewed changes

lo1tuma merged commit 51b3264 into lo1tuma:master Oct 3, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'one-suite-per-file' rule (#103) #105

Add 'one-suite-per-file' rule (#103) #105

alecxe commented Sep 28, 2016

lo1tuma left a comment

lo1tuma Sep 29, 2016

alecxe Sep 29, 2016

lo1tuma Sep 29, 2016

alecxe Sep 29, 2016

lo1tuma Sep 29, 2016

alecxe Sep 29, 2016

lo1tuma Sep 29, 2016 •

edited

Loading

alecxe Sep 29, 2016

alecxe Sep 29, 2016 •

edited

Loading

lo1tuma Sep 29, 2016

alecxe commented Sep 29, 2016

lo1tuma Sep 30, 2016

alecxe Sep 30, 2016

jfmengels left a comment

jfmengels Sep 30, 2016

alecxe Sep 30, 2016 •

edited

Loading

alecxe commented Sep 30, 2016

jfmengels commented Sep 30, 2016

alecxe commented Sep 30, 2016

alecxe commented Sep 30, 2016

jfmengels left a comment

jfmengels Sep 30, 2016

alecxe Sep 30, 2016

jfmengels Sep 30, 2016

jfmengels commented Sep 30, 2016

lo1tuma left a comment

alecxe commented Oct 3, 2016

jfmengels commented Oct 3, 2016

Add 'one-suite-per-file' rule (#103) #105

Add 'one-suite-per-file' rule (#103) #105

Conversation

alecxe commented Sep 28, 2016

lo1tuma 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lo1tuma Sep 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecxe Sep 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecxe commented Sep 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfmengels left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecxe Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

alecxe commented Sep 30, 2016

jfmengels commented Sep 30, 2016

alecxe commented Sep 30, 2016

alecxe commented Sep 30, 2016

jfmengels 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

jfmengels commented Sep 30, 2016

lo1tuma left a comment

Choose a reason for hiding this comment

alecxe commented Oct 3, 2016

jfmengels commented Oct 3, 2016

lo1tuma Sep 29, 2016 •

edited

Loading

alecxe Sep 29, 2016 •

edited

Loading

alecxe Sep 30, 2016 •

edited

Loading