New rule: no-hooks-from-ancestor-modules (#93) #94

raycohen · 2020-08-10T13:31:35Z

No description provided.

coveralls · 2020-08-10T13:35:07Z

Coverage remained the same at 100.0% when pulling c80c183 on raycohen:no-hooks-from-ancestor-modules into b53470d on platinumazure:master.

platinumazure

Hi @raycohen, thanks for contributing!

This is a good first pass, but there are a few things that could be improved:

First, the documentation's examples are a little confusing, because there are extra hook calls that aren't actually impacted by the rule-- but this could be confusing to the reader, who wants to know what lines will or won't warn based on how they invoke hooks. I've left some comments.

At a high level, just understand that it's more important to show the ESLint workings in this documentation, rather than show sensible/typical QUnit usage. (For example, of course an ancestor module that declares hooks would use them at the outer level before declaring the inner module-- but that's not important for the ESLint rule and it could just get in the way of the user trying to understand this rule.)

Also, this could use some more test cases. Please add some test cases involving:

Use of multiple hooks in a single module level (including multiple uses of the same hook, such as multple beforeEach calls)
Three levels of modules
Anything else you can think of that would show the rule as being more robust 😄

Thanks again for contributing, I really appreciate it! Let me know if you have any questions on this review.

platinumazure · 2020-08-11T17:26:50Z

docs/rules/no-hooks-from-ancestor-modules.md

+```js
+
+QUnit.module("outer module", function(hooks) {
+  hooks.beforeEach(function() {});


This line should not warn, so it might be good to simplify this example by removing this line (or maybe just adding a comment saying no warning would appear on this line). That way, the inner hooks.beforeEach is the only code that a reader would interpret as having a lint error.

platinumazure · 2020-08-11T17:27:09Z

docs/rules/no-hooks-from-ancestor-modules.md

+
+
+QUnit.module("outer module", function(outerModuleHooks) {
+  outerModuleHooks.beforeEach(function() {});


This line should not warn, so it might be good to simplify this example by removing this line (or maybe just adding a comment saying no warning would appear on this line). That way, the inner outerHooks.beforeEach is the only code that a reader would interpret as having a lint error.

platinumazure · 2020-08-11T17:28:02Z

docs/rules/no-hooks-from-ancestor-modules.md

+  outerModuleHooks.beforeEach(function() {});
+
+  QUnit.module("inner module", function(innerModuleHooks) {
+    innerModuleHooks.beforeEach(function() {});


This line should not warn, so it might be good to simplify this example by removing this line (or maybe just adding a comment saying no warning would appear on this line). That way, the outerModuleHooks.beforeEach is the only code that a reader would interpret as having a lint error.

docs/rules/no-hooks-from-ancestor-modules.md

platinumazure · 2020-08-11T17:34:08Z

lib/rules/no-hooks-from-ancestor-modules.js

+        },
+        fixable: null,
+        messages: {
+            noHooksFromAncestorModules: "Do not use hooks from an ancestor module."


This message is fine, but since you're passing a context, you might as well use it here!

I would suggest using the full text of the hook call (use context.getSourceCode().getText() on the callee node of the CallExpression) and use a message similar to the following: "{{hook}} is from an ancestor module."

What do you think?

platinumazure · 2020-08-11T17:38:59Z

lib/rules/no-hooks-from-ancestor-modules.js

+            "CallExpression:exit": function (node) {
+                if (utils.isModule(node.callee)) {
+                    contextStack.pop();
+                    hooksStack.pop();


This doesn't look correct. It's possible that a module could use multiple hooks (e.g., beforeEach and afterEach), so having only one pop here probably won't always work.

I would suggest having another if branch checking to see if the CallExpression being exited is a hook, and popping the hooksStack at that point.

With the current code, you could write some tests involving both beforeEach and afterEach hooks in the same module. When you run those, you will probably see some weird behavior with regard to the lint message. You can then use those tests to be more confident that your state management is good. 😄

I think naming is the issue here - I pushed updates where I tried to clarify what I am storing in the stack and when

raycohen · 2020-08-16T18:06:07Z

@platinumazure thanks for the feedback, I took another pass. Let me know what you think

tests/lib/rules/no-hooks-from-ancestor-modules.js

platinumazure

Nice improvements! Just a few things left.

Regarding duplicating tests for module calls-- I think it's okay if you just have one or two tests that use module, rather than duplicating all of them. Thanks!

docs/rules/no-hooks-from-ancestor-modules.md

lib/rules/no-hooks-from-ancestor-modules.js

platinumazure · 2020-08-27T22:07:12Z

lib/rules/no-hooks-from-ancestor-modules.js

+        },
+        fixable: null,
+        messages: {
+            "noHooksFromAncestorModules": "Do not use hooks from an ancestor module. You called {{usedHookIdentifierName}}.{{hookName}} inside module({{containingModuleDescription}})."


I love the greater level of detail here! ...Now, unfortunately, I need to ask if we can shorten the message. 😅

Let me know if you need help thinking of how you could shorten the message!

I could drop the "inside module(...)" portion, or are you looking for even shorter?

Even shorter might be better! If we want to mention the module's description, maybe we could do something like this?

Suggested change

"noHooksFromAncestorModules": "Do not use hooks from an ancestor module. You called {{usedHookIdentifierName}}.{{hookName}} inside module({{containingModuleDescription}})."

"noHooksFromAncestorModules": "Do not use `{{usedHookIdentifierName}}.{{hookName}}` from module({{containingModuleDescription}}) inside nested module."

I've shortened the message even more - since you'll see where the warning is, there's probably no extra value to the description of the containing module.

platinumazure · 2020-11-26T23:54:03Z

Friendly ping @raycohen, are you still interested in getting this landed? No rush, just checking 😄

raycohen · 2020-11-26T23:59:58Z

Yes, just got back from parental leave so I will try to get to this

raycohen · 2021-02-05T03:10:02Z

Made some updates. I wasn't sure which config this should go in. I went with 2️⃣ as my guess.

bmish · 2021-02-05T21:27:02Z

tests/lib/rules/no-hooks-from-ancestor-modules.js

+// Helpers
+//------------------------------------------------------------------------------
+
+function createError({ invokedMethodName, expectedHookIdentifierName, usedHookIdentifierName, containingModuleDescription }) {


You can also add some plain unit tests for this function at the bottom of this file.

For what it's worth, I don't think it's necessary to test a one-statement helper function in the test when it's pretty clear what it should do.

Oh true. My goal was to ensure the final error string looks right. But now I realized the string is not actually exposed here.

I'll clean up the things I ended up not using

clean up unused strings

platinumazure

Looks good so far. Thanks for helping to resolve the open questions!

Only real issue at this point is the inclusion of the rule in any configurations. We need to avoid modifying the shared configurations in semver-minor releases, which this PR would otherwise fall under. Please either remove the rule from the configurations, or let me know that you're okay with this being marked as a semver-major breaking PR. Thanks!

platinumazure · 2021-02-07T04:19:56Z

README.md

@@ -47,6 +47,7 @@ Each rule has emojis denoting:
 | :two: | [no-global-expect](./docs/rules/no-global-expect.md) | disallow global expect|
 | :two: | [no-global-module-test](./docs/rules/no-global-module-test.md) | disallow global module/test/asyncTest|
 | :two: | [no-global-stop-start](./docs/rules/no-global-stop-start.md) | disallow global stop/start|
+| :two: | [no-hooks-from-ancestor-modules](./docs/rules/no-hooks-from-ancestor-modules.md) | disallow the use of hooks from ancestor modules|


As far as configs go, I think this makes sense to have in recommended and two (eventually-- see next paragraph).

We can't make changes to the exported configs until a major release per the semver policy. If you're okay with waiting until a major release for this rule to go in, that's fine, but I don't know when that will be.

I recommend not including this rule in a config in this PR; we can release this as a minor release, and then there should be an issue for v6 planning and we could make sure this gets added into recommended and two configs in the major release. Let me know what you think.

platinumazure · 2021-02-07T04:21:40Z

index.js

@@ -43,6 +43,7 @@ module.exports = {
                "qunit/no-global-expect": "error",
                "qunit/no-global-module-test": "error",
                "qunit/no-global-stop-start": "error",
+                "qunit/no-hooks-from-ancestor-modules": "error",


As noted in another comment, we can't make changes to the exported configs until a major release. Also, I think this makes sense to include in both "recommended" and "two" (in the next major release). But for this PR, please remove this line (unless you are okay with this going in the next major release).

platinumazure · 2021-02-07T04:25:45Z

tests/lib/rules/no-hooks-from-ancestor-modules.js

+// Helpers
+//------------------------------------------------------------------------------
+
+function createError({ invokedMethodName, expectedHookIdentifierName, usedHookIdentifierName, containingModuleDescription }) {


For what it's worth, I don't think it's necessary to test a one-statement helper function in the test when it's pretty clear what it should do.

platinumazure

Just one possible issue in the documentation examples, otherwise this looks ready to go! Thanks for all the hard work on this!

docs/rules/no-hooks-from-ancestor-modules.md

platinumazure · 2021-02-13T22:13:52Z

Also, feel free to just push new commits on this branch-- makes it easier to review just the most recent changes. No need to squash and force push as I can do that on merge. Thanks!

Co-authored-by: Kevin Partington <platinumazure@gmail.com>

platinumazure

Looks awesome, thanks! I'll merge ASAP.

platinumazure · 2021-02-13T22:40:00Z

Thanks for your persistence on this one @raycohen!

raycohen force-pushed the no-hooks-from-ancestor-modules branch from 2fdd644 to 3889d07 Compare August 10, 2020 13:43

platinumazure requested changes Aug 11, 2020

View reviewed changes

raycohen force-pushed the no-hooks-from-ancestor-modules branch 2 times, most recently from b231361 to 5e9cbde Compare August 16, 2020 18:04

raycohen commented Aug 20, 2020

View reviewed changes

tests/lib/rules/no-hooks-from-ancestor-modules.js Show resolved Hide resolved

platinumazure requested changes Aug 27, 2020

View reviewed changes

raycohen force-pushed the no-hooks-from-ancestor-modules branch 5 times, most recently from 7ac9109 to 3139573 Compare February 5, 2021 03:06

bmish reviewed Feb 5, 2021

View reviewed changes

platinumazure requested changes Feb 7, 2021

View reviewed changes

raycohen force-pushed the no-hooks-from-ancestor-modules branch from 3139573 to c0f316d Compare February 7, 2021 16:18

New rule: no-hooks-from-ancestor-modules (qunitjs#93)

b612727

raycohen force-pushed the no-hooks-from-ancestor-modules branch from c0f316d to b612727 Compare February 7, 2021 16:35

platinumazure requested changes Feb 13, 2021

View reviewed changes

docs/rules/no-hooks-from-ancestor-modules.md Outdated Show resolved Hide resolved

Update docs/rules/no-hooks-from-ancestor-modules.md

c80c183

Co-authored-by: Kevin Partington <platinumazure@gmail.com>

platinumazure approved these changes Feb 13, 2021

View reviewed changes

platinumazure merged commit 379df8f into qunitjs:master Feb 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New rule: no-hooks-from-ancestor-modules (#93) #94

New rule: no-hooks-from-ancestor-modules (#93) #94

raycohen commented Aug 10, 2020

coveralls commented Aug 10, 2020 •

edited

Loading

platinumazure left a comment

platinumazure Aug 11, 2020

platinumazure Aug 11, 2020

platinumazure Aug 11, 2020

platinumazure Aug 11, 2020

platinumazure Aug 11, 2020

raycohen Aug 16, 2020

raycohen commented Aug 16, 2020

platinumazure left a comment

platinumazure Aug 27, 2020

raycohen Aug 27, 2020

platinumazure Aug 28, 2020

raycohen Feb 5, 2021

platinumazure commented Nov 26, 2020

raycohen commented Nov 26, 2020

raycohen commented Feb 5, 2021

bmish Feb 5, 2021

platinumazure Feb 7, 2021

bmish Feb 7, 2021

raycohen Feb 7, 2021 •

edited

Loading

platinumazure left a comment

platinumazure Feb 7, 2021

platinumazure Feb 7, 2021

raycohen Feb 7, 2021

platinumazure Feb 7, 2021

platinumazure left a comment

platinumazure commented Feb 13, 2021

platinumazure left a comment

platinumazure commented Feb 13, 2021



		QUnit.module("outer module", function(outerModuleHooks) {
		outerModuleHooks.beforeEach(function() {});

	"noHooksFromAncestorModules": "Do not use hooks from an ancestor module. You called {{usedHookIdentifierName}}.{{hookName}} inside module({{containingModuleDescription}})."
	"noHooksFromAncestorModules": "Do not use `{{usedHookIdentifierName}}.{{hookName}}` from module({{containingModuleDescription}}) inside nested module."

New rule: no-hooks-from-ancestor-modules (#93) #94

New rule: no-hooks-from-ancestor-modules (#93) #94

Conversation

raycohen commented Aug 10, 2020

coveralls commented Aug 10, 2020 • edited Loading

platinumazure 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

raycohen commented Aug 16, 2020

platinumazure 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

platinumazure commented Nov 26, 2020

raycohen commented Nov 26, 2020

raycohen commented Feb 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raycohen Feb 7, 2021 • edited Loading

Choose a reason for hiding this comment

platinumazure 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

platinumazure left a comment

Choose a reason for hiding this comment

platinumazure commented Feb 13, 2021

platinumazure left a comment

Choose a reason for hiding this comment

platinumazure commented Feb 13, 2021

coveralls commented Aug 10, 2020 •

edited

Loading

raycohen Feb 7, 2021 •

edited

Loading