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.

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

add allowFunctions as an allow-list for require-expect function calls #233

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions docs/rules/require-expect.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,18 @@ test. This rule checks for `expect` at linting time.

## Options

This rule is configurable by the `Check Type` and the `Config Options`

### Check Type

Example:

```js
rules: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

In order for eslint-plugin-markdown to pass, please surround this object with curly braces and remove the dangling comma at the end. Thanks!

'qunit/require-expect': ['error', 'except-simple'],
},
```

The "always" option requires that `expect` is called in each test.

The "except-simple" (**default**) option only requires an `expect` call when an assertion is
Expand All @@ -28,6 +40,28 @@ The "never-except-zero" option disallows `except` calls, except when used to
explicitly assert that a test performs no assertions, which would otherwise
be considered an error.

### Config Options

This rule allows exactly one config option: `allowFunctions`

#### allowFunctions

If you are using the Check Type `except-simple` whenever you pass `assert` into
another function you will cause an error. If you have some functions that you
are passing `assert` to that don't add to the number of asserts (and maybe just
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the part in parentheses is probably unnecessary. Would you mind removing it?

extract the test name from the assert context) then you can mark those functions
to be ok with the `allowFunctions` config option.

Here is an example:

```js
rules: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

In order for eslint-plugin-markdown to pass, please surround this object with curly braces and remove the dangling comma at the end. Thanks!

'qunit/require-expect': ['error', 'except-simple', {
allowFunctions: [ 'percySnapshot' ]
}],
},
```

## Rule Details

### always
Expand Down
15 changes: 15 additions & 0 deletions lib/rules/require-expect.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,15 @@ module.exports = {
schema: [
{
"enum": ["always", "except-simple", "never", "never-except-zero"]
},
{
"type": "object",
"properties": {
"allowFunctions": {
"type": "array"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add to this to require that all items in the array should be strings? Also, can we only allow this option if the previous value is except-simple?

}
},
"additionalProperties": false
}
]
},
Expand Down Expand Up @@ -67,10 +76,16 @@ module.exports = {
}

function isPassingAssertAsArgument(node) {
const allowedFunctions = context.options[1] && context.options[1].allowFunctions || [];

if (!currentTest.assertName) {
return false;
}

if (allowedFunctions.includes(node.callee.name)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will only work with identifiers. Do we want to support property accesses (for example, myLib.preprocess(assert)?

return false;
}

for (let i = 0; i < node.arguments.length; i++) {
if (node.arguments[i].name === currentTest.assertName) {
return true;
Expand Down
10 changes: 10 additions & 0 deletions tests/lib/rules/require-expect.js
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,16 @@ ruleTester.run("require-expect", rule, {
{
code: "test('name', function(assert) { assert.expect(0) });",
options: ["never-except-zero"]
},

// Sending assert to a function with a valid allowFunctions
{
code: [
"function myAssertion(a, assert, c) { assert.ok(true); }",
"test('name', function(assert) { myAssertion(null, assert, null); });"
].join(returnAndIndent),
options: ["except-simple", { allowFunctions: ["myAssertion"] }],
errors: [exceptSimpleErrorMessage("assert.expect")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this code pass? Please remove the errors property.

}
],

Expand Down