This repository has been archived by the owner on Aug 18, 2020. It is now read-only.

Add `no-vague-titles` rule #93

Merged

cartogram merged 1 commit into master from no-tests-contain-correct

Jun 24, 2018

Contributor

cartogram commented May 20, 2018 •

edited

Addresses one of the points here #77 by testing the it, describe and test statements against a simple regex that looks for the word "correct". Will fill in the docs once the general approach is approved.

cartogram requested a review from lemonmade

May 20, 2018 05:50

Contributor Author

cartogram commented May 20, 2018

Should we nest this rule inside of a "jest" folder (like with "webpack")?

Member

GoodForOneFare commented May 20, 2018 •

edited

So much potential 😍 Some suggestions:

Rename to be more general (e.g., no-weasel-words, no-ambiguous-titles, no-vague-titles)
Forbid appropriate
Forbid double-negatives
Submit this to eslint-plugin-jest

The double-negatives thing is maybe its own rule, but it could reuse code from this one.

Mocha has a similar rule: https://github.com/lo1tuma/eslint-plugin-mocha/blob/master/docs/rules/valid-test-description.md#rule-details.

lemonmade suggested changes

View reviewed changes

Member

lemonmade left a comment

Like Gord's suggestion of expanding this to be more of a "no-vague-titles" rule. Also all for nesting under jest/

lib/rules/no-tests-contain-correct.js Outdated

+                  const ignoredFunctionNames = ignore.reduce((accumulator, value) => {
+                    accumulator[value] = true;
+                    return accumulator;
+                  }, Object.create(null));

Member

lemonmade May 21, 2018

Probably better as a map, a little easier to construct too

lib/rules/no-tests-contain-correct.js Outdated

+                );
+              }
+              function isItDescription(node) {

Member

lemonmade May 21, 2018

This function name is confusing to me

lib/rules/no-tests-contain-correct.js Outdated

+              function descriptionContainsCorrect(node) {
+                if (isItTestOrDescribeFunction(node) && isItDescription(node)) {
+                  const description = testDescription(node);
+                  if (!description[0]) {

Member

lemonmade May 21, 2018

Checking if empty string?

lib/rules/no-tests-contain-correct.js Outdated

+                  }
+                  if (description.match(/correct/gi)) {
+                    return node.callee.name;

Member

lemonmade May 21, 2018

Why return the name?

lib/rules/no-tests-contain-correct.js Outdated

+                    CallExpression(node) {
+                      const erroneousMethod = descriptionContainsCorrect(node);
+                      if (erroneousMethod && !isIgnoredFunctionName(node)) {

Member

lemonmade May 21, 2018

Seems like a weird flow to me — I'd expect it to check if this is one of the functions we expect, removing ignored ones, then check if the description contains invalid words, then report.

lib/rules/no-tests-contain-correct.js Outdated

+                },
+              };
+              function isItTestOrDescribeFunction(node) {

Member

lemonmade May 21, 2018

Probably good to catch the skipped versions (fit/ it.only and the same for the other kinds)

cartogram force-pushed the no-tests-contain-correct branch 2 times, most recently from cf6a7d0 to c64fd75 Compare

May 23, 2018 03:45

cartogram changed the title ~~Add no-tests-contain-correct rule~~ Add no-vague-titles rule

cartogram force-pushed the no-tests-contain-correct branch 3 times, most recently from 51ece6e to 30facbd Compare

May 27, 2018 20:26

Contributor Author

cartogram commented May 27, 2018 •

edited

@lemonmade @GoodForOneFare thanks for the feedback! I have addressed everything and think this is ready for another look.

lemonmade suggested changes

View reviewed changes

docs/rules/jest/no-vague-titles.md Outdated

		@@ -0,0 +1,53 @@
		# Prevent the usage of vague words in test statements.(no-vague-titles)
		This rule prevents the use of the words "correct" and "appropriate" in test descriptions.

Member

lemonmade May 28, 2018

Make this a little more general

docs/rules/jest/no-vague-titles.md Outdated

		@@ -0,0 +1,53 @@
		# Prevent the usage of vague words in test statements.(no-vague-titles)

Member

lemonmade May 28, 2018

Space after period

docs/rules/jest/no-vague-titles.md Outdated


		## Rule Details

		Using vague words to convey the valid test path within `it`, `describe` or `test` descriptions often fails to communicate the meaningful details of what the underlying code is meant to do. Enabling this rule will encourage developers to write more specific and readable test descriptions.

Member

lemonmade May 28, 2018

Not sure what to convey the valid test path within means?

docs/rules/jest/no-vague-titles.md Outdated

		Examples of incorrect code for this rule:

		```js

Member

lemonmade May 28, 2018

Why do you leave empty lines in these?

lib/rules/jest/no-vague-titles.js Outdated

+                  function validate(node) {
+                    if (
+                      !isTestFunction(node) ||
+                      isIgnoredFunctionName(node) ||

Member

lemonmade May 28, 2018

I would probably flip this condition so they are all either positive or negative

lib/rules/jest/no-vague-titles.js Outdated

+                  return firstArgument.value;
+                }
+                return firstArgument.quasis[0].value.raw;

Member

lemonmade May 28, 2018

This is a little odd, might want to merge together all the quasis that you can to come up with a complete description

lib/rules/jest/no-vague-titles.js Outdated

+                const type = firstArgument.type;
+                if (type === 'Literal') {
+                  return firstArgument.value;

Member

lemonmade May 28, 2018

What if it's not a string literal?

lib/rules/jest/no-vague-titles.js Outdated

+              }
+              function containsVagueWord(description) {
+                return description.match(/correct/giu) || description.match(/appropriate/giu);

Member

lemonmade May 28, 2018

No need for g, we also don't usually bother with u

tests/lib/rules/jest/no-vague-titles.js Outdated

+                    parser,
+                  },
+                  {
+                    code: `test('foo bar baz')`,

Member

lemonmade May 28, 2018

You should also test non-test functions called with an argument that contains the bad words

tests/lib/rules/jest/no-vague-titles.js

+                    errors: errorWithMethod('describe'),
+                  },
+                  {
+                    code: "describe('correct')",

Member

lemonmade May 28, 2018

I would ignore for now, but you might be interested in tackling the new describe/it/test.each Jest recently introduced: https://facebook.github.io/jest/docs/en/api.html#describeeachtable-name-fn

cartogram force-pushed the no-tests-contain-correct branch 7 times, most recently from 77c1be6 to 242798e Compare

June 4, 2018 03:58

Contributor Author

cartogram commented Jun 4, 2018

Thanks again @lemonmade! Last round should all be addressed, let me know if there is anything else or good to merge.

lemonmade approved these changes

View reviewed changes

Member

lemonmade left a comment

Still think you can skip the member expression check.

lib/rules/jest/no-vague-titles.js Outdated

+                  }
+                  return {
+                    MemberExpression(node) {

Member

lemonmade Jun 4, 2018

But validate always works for call expressions, regardless of whether the callee is just an identifier or a member expression. You end up basically validating every member expression twice (once at the call expression, once at the member expression)

lib/rules/jest/no-vague-titles.js Outdated

+              }
+              function getDescription(node) {
+                const args = node.arguments || node.parent.arguments || [];

Member

lemonmade Jun 4, 2018

This is only needed because you are unnecessarily validating the member expression

cartogram force-pushed the no-tests-contain-correct branch 3 times, most recently from fa6bca2 to 66e676d Compare

June 16, 2018 17:36


          Add no-tests-contain-correct rule

0827edc

review feedback

docs

minor tweaks

more review fixes

Removing MemberExpression validation

cartogram force-pushed the no-tests-contain-correct branch from 66e676d to 0827edc Compare

June 16, 2018 17:39

cartogram mentioned this pull request

Support describe.each syntax in no-vague-titles #101

Closed

cartogram merged commit ef53ab1 into master

cartogram deleted the no-tests-contain-correct branch

June 24, 2018 20:49

cartogram deployed to production

July 16, 2018 08:03

Active

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.