toBeVisible should imply toBeInTheDocument

[calebeby]

Describe the feature you'd like:

I think that it would make sense for toBeVisible to perform, in addition to its CSS-based checks, checks that confirm that the element is in the document.

test('toBeVisible with removed element', () => {
  document.body.innerHTML = "<div id='el'>content</div>"
  const el = document.getElementById('el')
  expect(el).toBeVisible()
  el.remove()
  expect(el).not.toBeVisible() // this line fails, element is "visible"
})

If we think about it from a CSS standpoint, yes, there is no CSS that is preventing the element from being visible. But from the user's perspective, the element is definitely not visible. For this reason I think it makes sense to change the behavior of .toBeVisible to include the checks from .toBeInTheDocument.

[gnapse]

Makes sense. Are you up to contribute this fix? If not I may get to it next week.

[calebeby]

Yep, I can make a PR. One other thing I have a question about:

.toBeInTheDocument has a special case for null values:

expect(null).not.toBeInTheDocument() // passes

Should .toBeVisible also have that case?

expect(null).not.toBeVisible()

[gnapse]

toBeInTheDocument has that special case because a common use is to use a queryBy* or even document.querySelector and if they fail to return what you queried for, .toBeInTheDocument's most expected behavior would be to indeed let it pass, because that means the queried element was not found.

However, I'd say toBeVisible should not do the same. Can you check if it is letting it pass today? I believe it isn't, and in that case even more so, it should not change its functionality in that way.

[calebeby]

Yeah right now expect(null).not.toBeVisible() does not pass.

[gbirke]

Patch #339 broke some Vue tests for me. The tests look like this:

const wrapper = mount( SomeComponent, {localVue} );
let myInput = wrapper.find( 'input#foo' );
expect( myInput.element ).toBeVisible();

With version 5.11.10 I get "Received element is not visible (element is not in the document)" errors.

Using Vue 2.6.11 with @vue/test-utils 1.1.3

[gnapse]

@gbirke what's mount and wrapper in your example above?

Normally this library is used alongside the corresponding testing-library for the frontend framework you use. If you're using Vue, then you'd be using https://github.com/testing-library/vue-testing-library, and if you are, then the way to render a component using that library looks different than in your example. At least according to what is documented in https://github.com/testing-library/vue-testing-library#a-basic-example

Can you clarify that before we can discuss this further?

[gnapse]

I re-read your message and deducted that this what you mention as @vue/test-utils. I'm not sure how this testing library works. But I am assuming the problem may be because this library does not mount its root element inside a document. In which case there's little we can do. jest-dom assumes elements are living inside an actual document.

It would help if you created a minimal repo in codesandbox to investigate.

[gbirke]

I have created a minimal setup with vue-cli, adding the unit test module. Then I installed jest-dom and wrote a test that checks if the h3 element is visible. You can find it here: https://codesandbox.io/s/fuhrf

If you add console.log to the test, you will see that h3.element.getRootNode({composed: true}) doesn't return the document node (instead, it returns a Vue node), causing const isInDocument = element.ownerDocument === element.getRootNode({composed: true}) in isVisible to fail.

Checking for visibility with jest-dom is the recommended way of doing this, accoring to the Vue testing documentation

[gnapse]

Looking at that linked documentation, shouldn't you be using isVisible?

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

const wrapper = mount(Foo)
expect(wrapper.isVisible()).toBe(true)
expect(wrapper.find('.is-not-visible').isVisible()).toBe(false)

Like I said, it seems that testing library operates in a way that is incompatible with how jest-dom works. jest-dom assumes and expects that the DOM structure being tested is attached to a host document.

[GadQ]

@gnapse Thank you! I wanted to report an issue, but you're totally right and it worked!

[gbirke]

Quoting from the linked documentation:

isVisible is deprecated and will be removed in future releases.
Consider a custom matcher such as those provided in jest-dom

That's why I set up the test the way it is. You can raise an issue with @vue/test-utils to un-deprecate isVisible or explain the situation with element.ownerDocument === element.getRootNode({composed: true}).

For now I'll just stick with version 5.11.9 of jest-dom. If there are important updates to jest-dom I might use the testing library with render instead of using mount, but as long as @vue/test-utils is the recommended way of testing vue components, I'd rather not.

[FaustXVI]

Hi all :)

I just discovered testing-library (which is awesome, thanks a lot for that) and was about to report an issue for the case expect(null).not.toBeVisible(). I know it has been decided here already, but I would like to state my case :)

I think this test should pass.
It makes sense to me : anything that is not in the document is, by definition, not visible right ?
And from the user point of view, they don't really care if it's the document or not as long as they can't see it.

Let me give you an exemple (which is a close version of my actual code). I want to code a button that makes a request that may take time, so I want to show a spinner while waiting for the answer.

For the moment, my test code looks like this (simplifying the server mocking part because it's not important) :

const server = mockServer();
userEvent.click(screen.getByRole("button"));
expect(screen.queryByRole("status")).toBeVisible();
server.answer(200);
expect(screen.queryByRole("status")).not.toBeInTheDocument();

It works fine but it make details that I don't want to bother with explicit.
I think the next version is more aligned with the Guiding Principles :

const server = mockServer();
userEvent.click(screen.getByRole("button"));
expect(screen.queryByRole("status")).toBeVisible();
server.answer(200);
expect(screen.queryByRole("status")).not.toBeVisible();

What do you think ?

Edit : to avoid a breaking change, maybe introducing a toBeInvisible is an option ? Thus having the following test :

const server = mockServer();
userEvent.click(screen.getByRole("button"));
expect(screen.queryByRole("status")).toBeVisible();
server.answer(200);
expect(screen.queryByRole("status")).toBeInvisible();

[calebeby]

@FaustXVI

It sounds like you are suggesting that expect(null).not.toBeVisible() should pass, so that you can pass the result of queryBy* matchers in, like you can with expect(null).not.toBeInTheDocument()

I think it might make sense to make that change, but it sounds like @gnapse thinks it shouldn't pass

[gnapse]

What I think is not something final. I'm willing to change my mind.

One thing I may never be on-board with is to make a toBeInvisible() matcher that does not behave the same as .not.toBeVisible(). If we do this, it has to be changing how expect(null).not.toBeVisible() works, even if it implies a breaking change.

I'm hesitant to do this, but not outright opposed, only on the fence. I'd like the opinion of @testing-library/core-maintainers about it.

[eps1lon]

In my opinion toBeVisible on elements that are not in the document, does not make any meaningful assertion. Stylesheets wouldn't even apply to the element so you'd be missing almost all styles that may apply to the visibility of the element.

[FaustXVI]

@eps1lon I think I just understood something : toBeVisible goal is to test styles ?
If so, I completely misunderstood it and my request, indeed, doesn't make sense.
My meaning is : can a human see it ?
The too definitions are close but definitely different on that specific edge-case : «is something not there visible ?».

Of course, I have my prefered definition :) Before arguing for it, i'd like to be sure I understood you correctly :)

[eps1lon]

toBeVisible goal is to test styles ?

In testing-library we usually go with "visible" = possibly perceivable + accessible by assistive technology. E.g. display: none is impossible to be perceivable and inaccessible. On the other hand zero-dimensions is impossible to be perceivable but still be considered visible because the element would be exposed to assistive technology. This has some nice implications for testing in JSDOM since it removes mismatch of assertions between JSDOM and real browsers (JSDOM has no layout i.e. most elements have zero dimensions).

This definition also means that inaccessible color combinations, opacity, possible occlusion etc. are considered "visible" since it'd be incredibly hard to implement (and maintain) but also not possible in JSDOM (which is a popular testing environment).

My meaning is : can a human see it ?

Not every human has the same vision. Not every environment allows the same vision.

[kentcdodds]

I'm with @eps1lon on this 👍

[Rodigee]

Quoting from the linked documentation:

isVisible is deprecated and will be removed in future releases.
Consider a custom matcher such as those provided in jest-dom

That's why I set up the test the way it is. You can raise an issue with @vue/test-utils to un-deprecate isVisible or explain the situation with element.ownerDocument === element.getRootNode({composed: true}).

For now I'll just stick with version 5.11.9 of jest-dom. If there are important updates to jest-dom I might use the testing library with render instead of using mount, but as long as @vue/test-utils is the recommended way of testing vue components, I'd rather not.

@gbirke vue-test-utils has un-deprecated isVisible(), so you can now use test-utils's isVisible() instead of jest-dom's toBeVisible(). See vuejs/vue-test-utils#1675

[nickmccurdy]

If you are using Vue, we recommend our Vue Testing Library package.

It seems to me like there isn't a consensus on implementing this and the underlying issues are in @vue/test-utils, so I'll close this.