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

toBeVisible() returning true but it should return false #116

Closed
jordyvandomselaar opened this issue Jun 11, 2019 · 12 comments
Closed

toBeVisible() returning true but it should return false #116

jordyvandomselaar opened this issue Jun 11, 2019 · 12 comments

Comments

@jordyvandomselaar
Copy link

  • jest-dom version: 3.5.0
  • node version:v11.9.0
  • npm version:6.7.0

We have an accordion that renders like so:

dwf

As you can see it defaults to being closed.

The elements inside should thus be hidden, but

expect(getByText('MC3')).not.toBeVisible();

Errors and says an element inside is visible.

The accordion library we're using is https://github.com/springload/react-accessible-accordion version 2.4.5

@gnapse
Copy link
Member

gnapse commented Jun 11, 2019

It would be helpful if you provide a reproducible example, maybe in code sandbox or a repo that can be cloned and tried live in a browser. Or at the very least help us dig into the details, since you have the example live in your codebase.

One reason I can think of is that this library hides the elements via css classes, but if these classes are not loaded in the jsdom document during tests, this will not work. Keep in mind that most methods of loading css stylesheets do not work out of the box in the virtual jsdom environment where tests run.

@jordyvandomselaar
Copy link
Author

@gnapse I'll see if I can cook up an example, it does use CSS classes like you mentioned. Is there a way around this?

@gnapse
Copy link
Member

gnapse commented Jun 12, 2019

If it uses classes, then indeed, this will not work unless that stylesheet is attached to the document in the jsdom environment where the tests are running. This is a common pitfall with all the style-related custom matchers of ours. See #113 and int particular the lengthy answer I gave, and more concretely, a part with the heading "Stylesheets in jest-dom". The problem raised in that issue is also related to this. I pointed there to how we attach a stylesheet in the tests of our own code here.

As you can see, it's not straightforward to do, and also, you need to have access to the entire stylesheet codebase. I guess there could be ways to also manually attach a stylesheet file instead, by dynamically adding a <link rel="stylesheet" href="/path/to/static/css/file.css" /> to the head of the document, but I'm not sure how to do it.

This has come up so many times, and even once someone proposed adding a helper to attach stylesheets to the dom in the context of the text. For starters, in your case, do you have access to that stylesheet, or is something that this library generates dynamically under the hood?

Another alternative is to test for the presence or not of certain class names using .toHaveClass, if you know the names and these are static and not dynamically generated. I know this is less than ideal, as testing for a class name is more indirect than actually testing if an element is actually visible according to its computed styles.

Anyway, I hope I gave you some pointers, but that's the best I can do. Let me know fi any of this helps, or you with all this in mind you have a suggestion on how jest-dom as a library could help others deal with these situations. This is something we may have to think how to solve or more easily deal with.

@jordyvandomselaar
Copy link
Author

@gnapse Thank you for the reply! It sorta makes sense now. The struggle is we're using styled-components which doesn't expose a stylesheet afaik 😓

I guess I'll try and find another way to do this.

Thanks again,
Jordy

@gnapse
Copy link
Member

gnapse commented Jun 17, 2019

If the styled components use generates in the test environment a somewhat predictable class name, you could check it with .toHaveClass. To be honest, .toHaveClass only supports exact class name matching right now, but maybe an improvement could be to make it support partial matching, as .toHaveAttribute does, so you could something like this:

expect(element).toHaveClass(expect.stringContaining('hidden'))

This would pass if the element has any class name that contains the substring "hidden". To be clear, the above is a hypothetical that I'm proposing to support. It does not yet work that way.

@jordyvandomselaar
Copy link
Author

jordyvandomselaar commented Jun 18, 2019

That would help immensely if Styled-components were kind enough to do that by default 😓

More often than not we don't have access to any of the styling of an external package unfortunately

@Loque18
Copy link

Loque18 commented Oct 29, 2021

Hey im going throught the same issue here, i want to test a component that should not be visibile bc it has a class called is-hidden that adds the property display: none, but however the test keeps saying that the component is visible, so i tried with expect(element).toHaveClass('is-hidden'); it worked, but this doesn't seem to be very good since this seems to be an implementation detail

@gnapse
Copy link
Member

gnapse commented Nov 1, 2021

Indeed. Not ideal. But as it's explained before, CSS stylesheets are not mounted in the headless jsdom browser environment, where you mount components in isolation. Unlike when running the entire app. You'd either need to make sure that the stylesheets are loaded (something that depends a lot on how you do your CSS), or you test these things via a more e2e-like environment (Cypress, playwright, etc.)

@dancvogel
Copy link

dancvogel commented Nov 12, 2021

I get the limitations with jsdom here, but, man, this feels against the ethos of Testing Library. Having to check for a class instead of "visibility" is a test that relies on how the dev implemented visibility and not "focus on tests that closely resemble how your web pages are interacted by the users". Unfortunate.

@gnapse
Copy link
Member

gnapse commented Nov 13, 2021

I get the limitations with jsdom here, but, man, this feels against the ethos of Testing Library. Having to check for a class instead of "visibility" is a test that relies on how the dev implemented visibility and not "focus on tests that closely resemble how your web pages are interacted by the users". Unfortunate.

I fully agree. Do you have suggestions about how to better deal with this? I'd love for it to be better, but I do not know what to do. Stylesheets are not mounted in jsdom when mounting components in isolation, as that's something that usually the app's build systems do when you build the actual assets that will get served in a real browser.

@andion
Copy link

andion commented Jan 25, 2022

It's obvious now, thanks for the explanation, @gnapse!

I have no suggestions on how to better deal with this, but maybe a small warning on the docs will help to avoid these confusions, I can send a proposal via PR if you like.

Edit: I've already seen your PR #428 which clarifies the visibility related concepts. Thanks!

@Yankovsky
Copy link

Just want to share that this library worked for me https://github.com/dferber90/jest-transform-css

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

No branches or pull requests

6 participants