Skip to content

docs: new utils commenting their approaches #217

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

Closed
wants to merge 1 commit into from
Closed

docs: new utils commenting their approaches #217

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
45 changes: 45 additions & 0 deletions lib/node-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,3 +181,48 @@ export function isRenderVariableDeclarator(

return false;
}

type GetTestingLibraryUtilImportArgs = {
context: any,
utilName: string,
extraModules?: string[],
};

export function getTestingLibraryUtilImport(options: GetTestingLibraryUtilImportArgs): null | string {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the method to find any testing library import from @testing-library/framework or custom module defined in ESLint shared settings. It just needs 3 args:

  • context: from the rule, so from there it can get the source code with context.getSourceCode() to find all imports in different formats, and to get context.settings with plugin options.
  • utilName: this is the actual Testing Library util you are looking for (e.g. "render", "screen", "waitFor")
  • extraModules: an optional one to cover user-event, as it can be imported from custom module or its own package as it doesn't belong to Testing Library frameworks. It could be useful in other cases too.


/* Here we need to:
- look for imports of shape: import { <utilName> } from '@testing-library/whatever';
- look for imports of shape: import { <utilName> as <alias> } from '@testing-library/whatever';
- look for imports of shape: import * as testingLibrary from '@testing-library/whatever';
- look for imports of shape: const render = require('@testing-library/whatever').render;

From 3rd case, this method can't return what's the imported name for `utilName` as it's contained within an object.
What should we do in that case? Return an object or array to indicate if `utilName` has been imported directly or
under a namespace?
*/
Comment on lines +193 to +202
Copy link
Member Author

Choose a reason for hiding this comment

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

Here you can see the first problem. With the current implementation I had, I found this:

Working fine if name imported from testing library

// node
import { screen } from '@testing-library/framework';
// ...

// rule implementation
const importedScreen = getTestingLibraryUtilImport({ context, utilName: 'screen' });
console.log(importedScreen) // 'screen'

Working fine if rename imported from testing library

// node
import { screen as testingScreen } from '@testing-library/framework';
// ...

// rule implementation
const importedScreen = getTestingLibraryUtilImport({ context, utilName: 'screen' });
console.log(importedScreen) // 'testingScreen'

But I don't know what to do here

// node
import * as testingLibrary from '@testing-library/framework';
// ...

// rule implementation
const importedScreen = getTestingLibraryUtilImport({ context, utilName: 'screen' });
console.log(importedScreen) // null

For the 3rd case, this method must return something else to indicate 'screen' has been imported, but its namespaced under testingLibrary object rather than imported directly. How should the method return that then?

Copy link
Collaborator

Choose a reason for hiding this comment

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

🤔
what about returning an object instead? in addition to the alias (that might be the object exported, an alias, or a full object name that contains all the exported stuff), a enum/flag indicating the type of export so then you know how to treat the string?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think this is probably the best option. It'll return an object including the imported name and the context of the import. I need to think what to use for the context of the import. I guess I can use same types from AST, so from the example before it would return:

// node
import { screen as testingScreen } from '@testing-library/framework';
// ...

// rule implementation
const importedScreen = getTestingLibraryUtilImport({ context, utilName: 'screen' });
console.log(importedScreen) // { Identifier: 'testingScreen', type: 'ImportSpecifier' }
// node
import * as testingLibrary from '@testing-library/framework';
// ...

// rule implementation
const importedScreen = getTestingLibraryUtilImport({ context, utilName: 'screen' });
console.log(importedScreen) // { Identifier: 'testingLibrary', type: 'ImportNamespaceSpecifier' }

Thoughts?



/*
Would be nice to be able to find everything from `context.getSourceCode()` but I haven't done that before.
This way, we don't have to look for different import methods in the rule implementation, and just call this method
before returning the object with corresponding AST selectors.
Comment on lines +206 to +208
Copy link
Member Author

Choose a reason for hiding this comment

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

At the beginning I was thinking to receive node and settings, but receiving nodes from the rule wouldn't be really useful as the rule would have to look for all different import patterns. Instead, I want this method to receive just context to get both source code and settings. The thing is I've never looked for code/node from source code so it's taking me more time than I thought.

I'm using this eslint-plugin-import rule implementation as reference: https://github.com/benmosher/eslint-plugin-import/blob/master/src/rules/no-duplicates.js

*/

return null;
}

export function getTestingLibraryRenderImport(context: any): null | string {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a necessary wrapper around getTestingLibraryUtilImport when looking for a render method. Every Testing Library util will be searched just by received utilName but render must be searched also from additional custom names defined in ESLint methods. This method will take care of that by iterating through render and all custom renders and looking for any import matching one of them.

The implementation should work fine already, though it can be improved of course.

const customRenders = context.settings['testing-library/custom-renders'] || [];
const possibleRenderOptions = ['render', ...customRenders];

let importedRenderName = null;

possibleRenderOptions.forEach((renderOption) => {
const importedRenderOption = getTestingLibraryUtilImport({ context, utilName: renderOption});
if (importedRenderOption) {
importedRenderName = importedRenderOption
}
})

return importedRenderName
Comment on lines +218 to +227
Copy link
Collaborator

Choose a reason for hiding this comment

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

a really minor suggestion, but if we switch from null to use undefined, we can take advantage of find and just do

Suggested change
let importedRenderName = null;
possibleRenderOptions.forEach((renderOption) => {
const importedRenderOption = getTestingLibraryUtilImport({ context, utilName: renderOption});
if (importedRenderOption) {
importedRenderName = importedRenderOption
}
})
return importedRenderName
const importedRenderName = possibleRenderOptions.find((renderOption) => getTestingLibraryUtilImport({ context, utilName: renderOption}));
return importedRenderName

the only difference is that in case of multiple matches (are we considering that? should we guard or care about that scenario?) find will return the first one but with forEach the last match will be returned

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah of course. Much better approach.

About multiple matches: I thought about that possibility. Technically it could happen, but there are so many things I had to consider that I'm gonna just return first match I find. It could happen with something like:

import { render } from '@testing-library/foo';
import { render as customRender } from 'test-utils';

For now we will consider "render" as the single found util method. We can see in the future other alternatives to handle this, but I don't think this is an usual use case.

}
5 changes: 5 additions & 0 deletions lib/rules/render-result-naming-convention.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { ESLintUtils, TSESTree } from '@typescript-eslint/experimental-utils';
import { getDocsUrl, hasTestingLibraryImportModule } from '../utils';
import {
getTestingLibraryRenderImport,
isCallExpression,
isIdentifier,
isImportSpecifier,
Expand Down Expand Up @@ -51,9 +52,12 @@ export default ESLintUtils.RuleCreator(getDocsUrl)({
let renderAlias: string | undefined;
let wildcardImportName: string | undefined;

const testingLibraryRenderName = getTestingLibraryRenderImport(context);
Copy link
Member Author

@Belco90 Belco90 Aug 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This is how I'm planning to use the new methods, so you can just do this:

create(context) {
  const testingLibraryRenderName = getTestingLibraryRenderImport(context);
  // now you have 'render' within this var if render found and imported from related Testing Library module

  return {
    [`VariableDeclarator CallExpression[callee.name=${testingLibraryRenderName}]`](node) {
      // here you don't need to check anything manually about render alias or imported properly anymore!
      // just implement the actual checks for the rule
    }
  }
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I really like this

Copy link
Member Author

Choose a reason for hiding this comment

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

Right? I hope I can implement this version, so it would be really easy to focus on specific rules implementations. This is why is taking me so long basically, I'm trying to 1) find proper approach and 2) figure how to implement it. There are couple of things I need to try with context.getSourceCode, I think I might have something.


return {
// check named imports
ImportDeclaration(node: TSESTree.ImportDeclaration) {
// <-- this check would disappear
if (!hasTestingLibraryImportModule(node)) {
return;
}
Expand All @@ -69,6 +73,7 @@ export default ESLintUtils.RuleCreator(getDocsUrl)({
},
// check wildcard imports
'ImportDeclaration ImportNamespaceSpecifier'(
// <-- this check would disappear
node: TSESTree.ImportNamespaceSpecifier
) {
if (
Expand Down
93 changes: 76 additions & 17 deletions tests/lib/rules/render-result-naming-convention.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,11 +97,24 @@ ruleTester.run(RULE_NAME, rule, {
const button = screen.getByText('some button');
});
`,
options: [
{
renderFunctions: ['customRender'],
},
],
settings: {
Copy link
Member Author

Choose a reason for hiding this comment

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

Tests updated with the new ESLint shared settings.

'testing-library/custom-renders': ['customRender'],
'testing-library/custom-modules': ['test-utils'],
},
},
{
code: `
Copy link
Member Author

Choose a reason for hiding this comment

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

There is a case not covered in tests which I forgot to bring here. What about these 2 cases?

    {
      code: `
        import { render } from '@testing-library/foo';

       const setup = () => render(<SomeComponent />);
        
        test('should not report straight destructured render result from custom module', () => {
          const { unmount } = setup();
        });
      `,
      settings: {
        'testing-library/custom-renders': ['setup'],
      },
    },

    {
      code: `
        import { customRender } from 'test-utils';
        
        test('should not report straight destructured render result from custom module', () => {
          const { unmount } = customRender(<SomeComponent />);
        });
      `,
      settings: {
        'testing-library/custom-renders': ['customRender'],
      },
    },

On the first example: "custom-renders" option could be understood as user wants to report setup as render alias, while it's not imported from TL. So getTestingLibraryRenderImport would return just "render" which is the imported render found, but in the rule implementation we would need to still look for those custom renders. I don't think we can automate this somehow with another util to find custom renders not directly imported. I think the most we can do is to add a new util method where you pass the node containing "setup" and found render method returned by getTestingLibraryRenderImport and the method just checks if "setup" (or whatever custom render received) is wrapping given render. This sounds to much for me, I'd leave this up to each rule implementation.

On the second example: "custom-renders" is setup but there is no "custom-modules" setup. Our new util methods won't be aware of this "customRender" as there is no custom module where it could be possibly imported. I think this is fine.

import { render, screen } from 'test-utils';

test('should not report straight destructured render result from custom module', () => {
const { unmount } = render(<SomeComponent />);
const button = screen.getByText('some button');
});
`,
settings: {
'testing-library/custom-renders': ['customRender'],
'testing-library/custom-modules': ['test-utils'],
},
},
{
code: `
Expand All @@ -112,11 +125,23 @@ ruleTester.run(RULE_NAME, rule, {
await view.findByRole('button');
});
`,
options: [
{
renderFunctions: ['customRender'],
},
],
settings: {
'testing-library/custom-renders': ['customRender'],
'testing-library/custom-modules': ['test-utils'],
},
},
{
code: `
import { render } from 'test-utils';

test('should not report render result called "view" from custom module', async () => {
const view = render();
await view.findByRole('button');
});
`,
settings: {
'testing-library/custom-modules': ['test-utils'],
},
},
{
code: `
Expand All @@ -127,11 +152,23 @@ ruleTester.run(RULE_NAME, rule, {
await utils.findByRole('button');
});
`,
options: [
{
renderFunctions: ['customRender'],
},
],
settings: {
'testing-library/custom-renders': ['customRender'],
'testing-library/custom-modules': ['test-utils'],
},
},
{
code: `
import { render } from 'test-utils';

test('should not report render result called "utils" from custom module', async () => {
const utils = render();
await utils.findByRole('button');
});
`,
settings: {
'testing-library/custom-modules': ['test-utils'],
},
},
{
code: `
Expand Down Expand Up @@ -327,11 +364,33 @@ ruleTester.run(RULE_NAME, rule, {
const button = wrapper.getByText('some button');
});
`,
options: [
settings: {
'testing-library/custom-renders': ['customRender'],
'testing-library/custom-modules': ['test-utils'],
},
errors: [
{
renderFunctions: ['customRender'],
messageId: 'invalidRenderResultName',
data: {
varName: 'wrapper',
},
line: 5,
column: 17,
},
],
},
{
code: `
import { render } from 'test-utils';

test('should report from custom render module ', () => {
const wrapper = render(<SomeComponent />);
const button = wrapper.getByText('some button');
});
`,
settings: {
'testing-library/custom-modules': ['test-utils'],
},
errors: [
{
messageId: 'invalidRenderResultName',
Expand Down