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.

Sign up for GitHub

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

Jump to bottom

docs(valid-describe): add suggestion to disable for dynamic tests #253

Closed
wants to merge 3 commits into from
Closed

docs(valid-describe): add suggestion to disable for dynamic tests #253

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a0d1b46
docs(valid-describe): add suggestion to disable for dynamic tests
chrisdothtml May 10, 2019
1902be0
fix: wording
chrisdothtml May 10, 2019
2f8b692
fix: tests
chrisdothtml May 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 19 additions & 2 deletions docs/rules/valid-describe.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,25 @@ errors.

## Rule Details

This rule validates that the second parameter of a `describe()` function is a
callback function. This callback function:
This rule validates the signature of `describe()` functions. It validates two
things:

#### 1. The first argument is a string literal

NOTE: if you are using a dynamic value, disable the rule inline like so:

```js
// eslint-disable-next-line jest/valid-describe
describe(myTestName, () => {
```

See
[this comment](https://github.com/jest-community/eslint-plugin-jest/pull/253#issuecomment-491371038)
for reasoning behind not supporting dynamic values.

#### 2. The second argument is a callback function

This callback function:

- should not be
[async](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function)
Expand Down
3 changes: 2 additions & 1 deletion rules/__tests__/valid-describe.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@ ruleTester.run('valid-describe', rule, {
code: 'describe(() => {})',
errors: [
{
message: 'First argument must be name',
message:
Copy link
Member

@SimenB SimenB May 11, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look at the code - this should keep the old message. Can we differentiate between missing first arg/wrong type and a variable?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I follow. Could you explain what exactly you're looking for?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the point is that the new message suggests that the mistake was to pass a function instead of a string, when the mistake is really that the name is just missing before the function. The old message was neutral in this regard.
And I guess @SimenB's suggestion (second sentence) was to print something like "First argument must be name" in this case because we can infer that the user probably just forgot the name in before the function, and only if both args are there but the first is not a string literal print the new one :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's what I meant. Thanks! 😀

'Name must be a string literal (disable this rule inline for dynamic tests)',
line: 1,
column: 10,
},
Expand Down
3 changes: 2 additions & 1 deletion rules/valid-describe.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ module.exports = {
const [, callbackFunction] = node.arguments;
if (!isString(name)) {
context.report({
message: 'First argument must be name',
message:
'Name must be a string literal (disable this rule inline for dynamic tests)',
loc: paramsLocation(node.arguments),
});
}
Expand Down