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

Rules do not work with custom name library name #173

Closed
stefcameron opened this issue Jun 17, 2020 · 21 comments
Closed

Rules do not work with custom name library name #173

stefcameron opened this issue Jun 17, 2020 · 21 comments
Labels
enhancement New feature or request
Milestone

Comments

@stefcameron
Copy link

I love the no-debug rule, however, it doesn't work if you're using a custom local library name in order to, for example, provide a custom render function to the rest of your code.

I agree that the library name is no longer @testing-library/react, for example, and could be anything, so the rule doesn't find a match, but because customizing the render function is in the docs, and I presume quite standard practice, it makes this rule an immediate non-starter, which is a shame since the rule would be really nice to have.

Seems like the rule needs an option, similar to the existing renderFunctions option, that we can use to specify the name of our import. In my case, it's import ... from 'testingUtility'.

import { screen } from '@testing-library/react';
...
screen.debug(); // IS a lint error

however,

import { screen } from 'testingUtility';
...
screen.debug(); // is NOT a lint error -- and should be
@timdeschryver
Copy link
Member

This probably affects most of the (all?) rules 😅

@Belco90
Copy link
Member

Belco90 commented Jun 18, 2020

Wow, didn't think about this before. @timdeschryver exactly, this can potentially affect all the rule. To solve this properly we would need to include a global param in the plugin config so you can set your custom module from where you import Testing Library stuff. Otherwise, we can't know if the screen you are importing is from Testing Library or not.

@Belco90 Belco90 added the enhancement New feature or request label Jun 18, 2020
@nickserv
Copy link
Member

nickserv commented Jun 18, 2020

Could we alternatively solve this without any config using some more dynamic logic?

@Belco90
Copy link
Member

Belco90 commented Jun 18, 2020

I don't know if it's possible to guess this from our ESLint component. I'll check how other libraries get this kind of dynamic values from custom user files.

@nickserv nickserv changed the title no-debug does not work with custom name library name rules do not work with custom name library name Jun 18, 2020
@nickserv nickserv changed the title rules do not work with custom name library name Rules do not work with custom name library name Jun 18, 2020
@timdeschryver
Copy link
Member

TIL, there are shared settings seems like something we can use? Not sure tho...

ESLint supports adding shared settings into configuration file. You can add settings object to ESLint configuration file and it will be supplied to every rule that will be executed. This may be useful if you are adding custom rules and want them to have access to the same information and be easily configurable.

@Belco90
Copy link
Member

Belco90 commented Jun 18, 2020

@timdeschryver Yes, that's the option I was thinking about. That's what I have to use for setting up eslint-plugin-import for example. Others like eslint-plugin-node can look for other files or settings, but I'm afraid we will have to do this with ESLint settings.

@nickserv
Copy link
Member

What's the difference? I thought they all used the ESLint config.

@Belco90
Copy link
Member

Belco90 commented Jun 18, 2020

What's the difference between what? Sorry, I don't know what you mean. ESLint setting is just a key within ESLint config file.

@stefcameron
Copy link
Author

I think ESLint Shared Settings, as @timdeschryver points out, would be a good way to go to solve this for all rules. Another example is eslint-plugin-react, see configuration options

@nickserv
Copy link
Member

@stefcameron Ah yes, that is what I was thinking of, thanks

@stefcameron
Copy link
Author

I also wanted to address a question about the use case I saw via email notification yesterday night, but can no longer find on this issue. Perhaps it was deleted, but I thought I would still clarify for posterity.

In my description of the issue, I mentioned the need for custom render functions, which is straight out of Testing Library documentation.

The documentation for the no-debug rule actually points to an example, which is an illustration of my use case, though not my exact use case (and also straight out of Testing Library docs): https://testing-library.com/docs/example-react-redux

My specific use case is related to using React context providers and the useContext hook. The concrete example is with Emotion Theming. The product I'm working on relies on the <ThemeProvider> to provide the theme object to any component.

The result is that nearly every component calls the useTheme() hook in some way or another, which means it cannot be rendered without being wrapped in <ThemeProvider>:

import { render, screen } from '@testing-library/react';
import { ThemeProvider } from 'emotion-theming';
import { lightTheme } from './themes';
import { Component } from './file';
...
it('should render', () => {
  render(
    <ThemeProvider theme={lightTheme}>
      <Component />
    </ThemeProvider>
  );
  expect(screen.getByRole('something')).toBeInTheDocument();
});

Therefore, I followed the example in the docs -- which happens to use a generic <ThemeProvider> 🙂 -- to "replace" the provided render() method with an enhanced version that wraps the component being tested with an instance of <ThemeProvider>.

I took it a step further and also configured Jest to remove the need to use crazy relative paths everywhere and just import ... from 'testingUtility';. So now my code becomes:

import { render, screen } from 'testingUtility';      // <- my local module that uses @testing-library/react internally
// * no need to import emotion-theming and themes
import { Component } from './file';
...
it('should render', () => {
  render(<Component />);                                      // <- much simpler!
  expect(screen.getByRole('something')).toBeInTheDocument();
});

Another advantage here is that if an additional context provider is needed later on, we can add it in one place (in testingUtility.js) and every single test suite will automatically get "upgraded" to use it.

So I think this is a very valid use case, especially since it's straight out of the Testing Library docs, and I think it should be supported, if possible.

@nickserv
Copy link
Member

nickserv commented Jun 18, 2020

I deleted my comment originally because I forgot that you mentioned the custom render function, but this does help to add some more clarity to different use cases, thanks! Another idea is that you might need to adjust TypeScript types for passed props/context/etc. by wrapping the testing library functions in types with different interfaces (this used to be the way to provide themes to Styled Components before they implemented type augmentation).

Given all these use cases, I guess we do need a config. We would have to resolve the modules to search the dependency tree for testing libraries, and even with that you couldn't easily publish a fork. It would also be difficult to accurately determine whether a module is meant to replace testing library, or is just some testing utility that happens to import a method from it (in which case you wouldn't want your linter telling you to use functions from it that don't exist).

I still think there's another problem even with a custom config though: The module wrapping testing library would have to have the same API with the same semantics, otherwise rules might recommend the wrong methods, methods may not exist, or advice may not be relevant. I guess there isn't really a way around this beyond telling users to write their own custom ESLint rules (which you can do locally without publishing a separate package), so we might want to warn that these rules are only designed to be used with a wrapper that follows the same API and semantics, and possibly link to the ESLint docs for custom rules.

@gndelia
Copy link
Collaborator

gndelia commented Jun 18, 2020

In the project where I work for we use a utility function as an intermediate to enhance render method with providers, but we also use it to re-export the rest of the RTL API - imports come from only one way.

But because of that, many rules will stop working because they rely on the specific import of RTL. I also believe this will be a common scenario, especially if it's recommended straight out of the docs.

Not sure if the config is the best way to fix it, but it seems the best option so far?

In our case the intermediate module do has the same semantic as RTL API, we should probable enforce that in the docs if we go the config way

@nickserv
Copy link
Member

I'm mostly concerned about what the docs recommend and what's common best practices, so if most are doing this, the config option should be fine.

@stefcameron
Copy link
Author

@nickmccurdy The way I was thinking of it, with the config option, is that someone would provide the alternate name for the React Testing Library (RTL) module being used. In my case, that would be "testingUtility".

I will guess that not everyone will configure Jest as I have, so they would have imports like this instead (for example):

import { render} from 'testingUtility'; // vs
 
import { render } from '../../../test/testingUtility'; // or
const { render } = require('../../../test/testingUtility');

In that case, I was thinking that it's still the module name that would matter, not the path. So you could configure the name as "testingUtility" and it would work in all cases above.

I also think it's completely reasonable to make the assumption that the module's interface is the same as (or a superset of) RTL's interface for this config option. So if there's a reference to render or debug from that module, the rules give those tokens the same connotation as if the import was directly from RTL.

(BTW, my local testingUtility is a superset of RTL because it also provides about 6 other alternate render methods for various use cases, some examples being lightThemeRender, darkThemeRender, as well as a docsRender which we use for unit testing our generated docs system that happens to use different providers. So have already used the config options for no-debug to let it know about these alternate names, and it works great, when the import is @testing-library/react, of course 😉 .)

Of course, it would be good to state this assumption as a warning for this config option.

@Belco90
Copy link
Member

Belco90 commented Jun 19, 2020

I think we also need to check default testing library imports (e.g. @testing-library/react) even if the user sets a custom testing library module through ESLint settings. In my code base, the custom module only expose alternate render methods. So if we want the default render, we import from testing library itself. But if we want a custom one, we import from our own module so we have mixed imports.

I don't think this makes the implementation more difficult, but we need to append the custom module to the list of modules to check, rather than check one or the other.

@VED-StuartMorris
Copy link

This looks to be associated with v4 which is not released yet, is that correct? What is the fix in place for this?

Will the import requires a specific naming pattern or will it work with any import that in turn imports @testing-library? We currently use the name test-utils.tsx not that changing it matters :)

@Belco90
Copy link
Member

Belco90 commented Feb 17, 2021

@VED-StuartMorris this has been solved in v4, not released yet indeed (it should be sooner than later).

This will be explained properly in the doc of the plugin, but basically the solution implemented is what we call Aggressive Reporting. With this opted-in mechanism, the plugin will report all utils found if matching Testing Library ones no matter where they come from. So if you call render in a test file, it will be taken into account for reporting no matter if coming from '@testing-library/react', 'test-utils' or 'foo'. In case this cause any issue in your codebase, there will be a new setting so you can opt-out this Aggressive Reporting and restrict the valid modules where Testing Library utils can be imported from. You'll be able to do this through ESLint shared settings.

In your case you will be able to do something like:

// your eslintrc file
{
  "settings": {
     "testing-library/custom-module": "test-utils"
  }
}

With this config and previous example, only render method coming from '@testing-library/react' or 'test-utils' would be reported. Those coming from another module like 'foo' would be ignored. Anyway, in your case you won't need to set this since Aggressive Reporting will take care of reporting that for you. This is just in case you need to restrict the scope for whatever reason.

@VED-StuartMorris
Copy link

Thanks @Belco90 that sounds great.

Just to clarify, this would only occur on .test. files? Furthermore, this would work on any import from the utils file?

We export all from @testing-library

export * from '@testing-library/react';

Then we would import { render, screen ... } from the util. So screen.debug would be picked up too.

@Belco90
Copy link
Member

Belco90 commented Feb 17, 2021

@VED-StuartMorris I have more good news for you! We have added another shared setting to customize on which files you want the plugin to be ran based on a regexp. By default it will report all files with suffix .test or .spec, so you don't need to set up anything if that works for you. However, let's say you are using .test files for Testing Library tests and .spec for Cypress tests, you can set this new shared setting to run the plugin only on .test files. You'll have more details and instructions on final v4 docs.

Furthermore, this would work on any import from the utils file?

Yes, basically for all utils matching Testing Library ones: queries, render, screen, waitFor, etc. So that example you mentioned will be automatically reported for you just by upgrading to v4.

@Belco90
Copy link
Member

Belco90 commented Apr 11, 2021

This should be fixed on v4.0.0

The release is available on:

@Belco90 Belco90 closed this as completed Apr 11, 2021
@MichaelDeBoey MichaelDeBoey added this to the v4 milestone Apr 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

7 participants