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

Adding optional settings to require-hook rule #983

Merged
merged 1 commit into from Nov 23, 2021

Conversation

Kreeg
Copy link
Contributor

@Kreeg Kreeg commented Nov 13, 2021

The problem

Some testing libs provide methods which takes hook as an argument and should be executed outside of a hook. The greatest example is using enableAutoDestroyHook from @vue/test-utils lib .
By default jest/require-hook rule throws an error in such correct code

import {
  enableAutoDestroy,
  resetAutoDestroyState,
  mount
} from '@vue/test-utils'
import Foo from './Foo.vue'

// calls wrapper.destroy() after each test
enableAutoDestroy(afterEach)
// resets auto-destroy after suite completes
afterAll(resetAutoDestroyState)

describe('Foo', () => {
  it('renders a div', () => {
    const wrapper = mount(Foo)
    expect(wrapper.contains('div')).toBe(true)
    // no need to call wrapper.destroy() here
  })
})

To deal with it you must ignore jest/require-hook rule (or mark it as warn). Which is absolutely wrong and causing all the problems described in original rule's docs.

The solution

In this PR I added addtional settings for the rule to exclude such tools as enableAutoDestroy with additional settings to reduce or maybe extend allowed hooks.

@Kreeg Kreeg force-pushed the adding-require-hook-options branch 3 times, most recently from 28486d4 to c33f78e Compare November 13, 2021 09:08
Copy link
Collaborator

@G-Rath G-Rath left a comment

Choose a reason for hiding this comment

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

I'm heading to bed so will give it a proper review tomorrow, but I'm thinking we should just have an allowedFunctions option, as I think functions taking hooks as their first argument is reasonably specific (I've never come across it myself until now), and if we check for this arguably there could be other patterns people could like us to support (like functions that take the hook as the last argument, if they exist)

@G-Rath
Copy link
Collaborator

G-Rath commented Nov 13, 2021

Btw I'm pretty sure you can call the method in a beforeAll and that that would technically be better as Jest would ensure its actually called in the right order.

But I think it's worth us supporting allowedFunctions anyway :)

@Kreeg
Copy link
Contributor Author

Kreeg commented Nov 13, 2021

Hey @G-Rath! Thanks for commenting.
Yes, I think that's reasonable to allow hooks to be not in specific order. But I'm sure it's good to validate that hooks are passed in arguments list, because I have not yet encountered with any valid function called outside a hook

The first idea came to me is

// helpers.js
export default doAllMocks = () => {
  jest.mock('oneModule');
  jest.mock('anotherModule');
  // list of 20 mocks
}

// test.js
import doAllMocks from './helpers';

doAllMocks();

But I'm not sure it is a good practice.

Btw I'm pretty sure you can call the method in a beforeAll and that that would technically be better as Jest would ensure its actually called in the right order.

Yes, technically it is good but for me it looks like a little overengineered, like calling afterAll hook inside beforeAll (for example using resetAutoDestroyState in @vue/test-utils) just for the linting to be passed.

Goodnight! I'll be waiting for any decisions 👍

@G-Rath
Copy link
Collaborator

G-Rath commented Nov 13, 2021

@Kreeg I still think we should go with a more generic option as it means less complexity on our side and if we wanted to make it more generic later that'd be a breaking change since we'd be removing an option.

e.g. it's perfectly valid (and easy) to do something like this:

export const setupAutoDestroy = () => {
  beforeAll(() => {
    enableAutoDestroy(afterEach);
  });
};

This would also make it easier to reuse e.g. for #961, when checking const assignments.

@Kreeg
Copy link
Contributor Author

Kreeg commented Nov 13, 2021

@G-Rath it seems reasonable for me.
I'll update PR tomorrow, now it's my time to go to bed :D

@Kreeg Kreeg force-pushed the adding-require-hook-options branch from c33f78e to 8405f54 Compare November 14, 2021 08:05
@Kreeg
Copy link
Contributor Author

Kreeg commented Nov 14, 2021

@G-Rath hey! I updated the branch. I personally don't like meaningless commits so I force pushed it.

@Kreeg Kreeg requested a review from G-Rath November 14, 2021 08:11
Copy link
Collaborator

@G-Rath G-Rath left a comment

Choose a reason for hiding this comment

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

Looks good - got a couple of minor changes I'd like to see; also, what do you think about using "allow" instead of exclude? e.g. allowedFunctionCalls.

I feel like that might be a better name since we're allowing functions to be called - exclude to me feels like we're bailing out of checking completely, which isn't the case (we're just not reporting them).

Comment on lines 154 to 158
Some test utils provides methods which takes hook as an argument
and should be executed outside a hook.

For example https://vue-test-utils.vuejs.org/api/#enableautodestroy-hook
which takes the hook as an argument. To exclude them you can update settings
Copy link
Collaborator

Choose a reason for hiding this comment

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

The fact that the methods take hooks as an argument is irrelevant, so this should be say something along the lines of "if there are methods that you want to call outside of hooks and tests, you can mark them as allowed using the allowedFunctionCalls option" :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the documentation but I still don't like it. Because the misleading option allowedFunctionCalls, which contain correct function, but the code example is still incorrect because of another function calls. Is it good?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Because the misleading option allowedFunctionCalls,

I'm not sure I follow you on what you're finding misleading?

I'm happy if you want to play with the documentation some more - do you think it would be better if we didn't have any config in our "incorrect" example (e.g. "Example of incorrect code without using allowedFunctionCalls"), and left the "correct" example as-is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@G-Rath yes, the problem is just with "incorrect" example. So in docs we are saying "incorrect usage of allowed functions" but there is no incorrect use of these functions in the example. This makes some confusion so I'll remove it

docs/rules/require-hook.md Outdated Show resolved Hide resolved
docs/rules/require-hook.md Outdated Show resolved Hide resolved
src/rules/__tests__/require-hook.test.ts Show resolved Hide resolved
Comment on lines 38 to 44
const nodeName = getNodeName(node);

if (nodeName === null) {
return false;
}

return !!options.excludedFunctions?.includes(nodeName);
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should be able to just cast this to a string, which makes the code a little smaller without any real loss (since this is a known TS limitation that we've got already in a few other places)

Suggested change
const nodeName = getNodeName(node);
if (nodeName === null) {
return false;
}
return !!options.excludedFunctions?.includes(nodeName);
return !!options.excludedFunctions?.includes(getNodeName(node) as string));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! This is my first encounter with TS.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No problem, you're doing good!

src/rules/require-hook.ts Outdated Show resolved Hide resolved
@Kreeg
Copy link
Contributor Author

Kreeg commented Nov 15, 2021

@G-Rath Saw your review, I will answer and update later tonight, thank you!

@Kreeg Kreeg force-pushed the adding-require-hook-options branch from 8405f54 to ae01775 Compare November 15, 2021 15:46
@Kreeg Kreeg requested a review from G-Rath November 15, 2021 15:48
@Kreeg Kreeg force-pushed the adding-require-hook-options branch from ae01775 to 71a0df0 Compare November 15, 2021 18:46
@Kreeg Kreeg force-pushed the adding-require-hook-options branch from 71a0df0 to 375069f Compare November 15, 2021 19:08
Copy link
Collaborator

@G-Rath G-Rath left a comment

Choose a reason for hiding this comment

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

Looking good! Got a few minor nits/recommendations, but nothing blocking - will merge sometime this week :)

src/rules/require-hook.ts Outdated Show resolved Hide resolved
src/rules/require-hook.ts Outdated Show resolved Hide resolved
src/rules/require-hook.ts Outdated Show resolved Hide resolved
docs/rules/require-hook.md Outdated Show resolved Hide resolved
docs/rules/require-hook.md Outdated Show resolved Hide resolved
@Kreeg Kreeg requested a review from G-Rath November 15, 2021 19:12
@Kreeg
Copy link
Contributor Author

Kreeg commented Nov 15, 2021

@G-Rath I will fix the code due to your suggestions tomorrow, thank you :)

@Kreeg Kreeg force-pushed the adding-require-hook-options branch from 375069f to 072b08a Compare November 16, 2021 15:24
Copy link
Collaborator

@G-Rath G-Rath left a comment

Choose a reason for hiding this comment

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

Thanks for this!

@G-Rath G-Rath merged commit 2a74bca into jest-community:main Nov 23, 2021
G-Rath pushed a commit that referenced this pull request Nov 23, 2021
Co-authored-by: Andrey Nelyubin <nelyubin_a_a@sunlight.net>
@G-Rath
Copy link
Collaborator

G-Rath commented Nov 23, 2021

(I used fix instead of feat, so manually re-worded the commit: 9d9336a 😅)

github-actions bot pushed a commit that referenced this pull request Nov 23, 2021
# [25.3.0](v25.2.4...v25.3.0) (2021-11-23)

### Features

* **require-hook:** add `allowedFunctionCalls` setting ([#983](#983)) ([9d9336a](9d9336a))
@Kreeg Kreeg deleted the adding-require-hook-options branch January 29, 2022 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants