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.

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

Jump to bottom

fix: update VTU to v1, update types #139

Merged
merged 10 commits into from May 20, 2020
Merged

fix: update VTU to v1, update types #139

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8231b0d
feat(@vue/test-utils): upgrade to v1
afenton90 May 6, 2020
1582bc6
refactor(vtu): upgrade to 1.0.2
afenton90 May 6, 2020
0ac8f26
Update build-related packages
afontcu May 13, 2020
8d76484
Remove removed sync option
afontcu May 16, 2020
13ba03e
Bump DOM Testing Library
afontcu May 16, 2020
d47df70
Bump to VTU 1.0.3
afontcu May 18, 2020
902d899
Fix Vuetify test
afontcu May 20, 2020
3831452
Remove test unrelated to Vuetify
afontcu May 20, 2020
d6087aa
Bump DTL patch version
afontcu May 20, 2020
dc38df6
Add @afenton90 as a contributor
afontcu May 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Expand Up @@ -43,7 +43,7 @@
"@babel/runtime": "^7.8.7",
"@testing-library/dom": "^7.0.2",
"@types/testing-library__vue": "*",
"@vue/test-utils": "^1.0.0-beta.32"
"@vue/test-utils": "^1.0.1"
},
"devDependencies": {
"@babel/plugin-transform-runtime": "^7.8.3",
Expand Down
2 changes: 1 addition & 1 deletion src/__tests__/vuetify.js
View file
Open in desktop
Expand Up @@ -69,5 +69,5 @@ test('opens a menu', async () => {
expect(menuItem).toBeInTheDocument()

await fireEvent.click(menuItem)
expect(queryByText('menu item')).not.toBeInTheDocument()
expect(queryByText('menu item')).not.toBeVisible()
Copy link
Member

Choose a reason for hiding this comment

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

Hm, did this test start to fail? 🤔 I mean, did Vuetify changed anything? I'm worried this is related to attachTo

Copy link
Contributor Author

@afenton90 afenton90 May 6, 2020 •
edited

Choose a reason for hiding this comment

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

Yeah it did start to fail. When I debugged though it seemed like once menu list was open it just added display none to hide it.
Let me debug with attachToDocument to see what it was outputting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So with master this works fine. Using debug shows that the menu item is removed from the DOM.
However, with the latest version of VTU and attachToDocument still show this in the DOM.

So looks like an issue with the latest release of VTU rather than use of attachTo

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, I spent some time on this. The breaking change is actually from vue/test-utils@1.0.0-beta.32 -> vue/test-utils@1.0.0-beta.33.

I will debug a bit more.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't ask how I figured this out but THIS LINE is the problem. Adding it back in fixes this test.

I do not know why this was removed or what this line does.

Copy link
Member

Choose a reason for hiding this comment

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

Hey! 👋 yeah, I'm currently looking into it. I started to think that toBeInTheDocument() was actually not the right assertion.

AFAICT, Vuetify does render the element but hides it, so toBeInTheDocument() should have passed. Now, I don't have much experience with Vuetify, so I might be wrong.

I am unable, though, to make this pass (and I think it should):

test('opens a menu', async () => {
  const {getByText, queryByText} = renderWithVuetify(VuetifyDemoComponent)

  const openMenuButton = getByText('open menu')

  // Menu item is initially not rendered
  expect(queryByText('menu item')).not.toBeInTheDocument()

  await fireEvent.click(openMenuButton)

  const menuItem = getByText('menu item')

  expect(menuItem).toBeInTheDocument()
  expect(menuItem).toBeVisible() // <-- this fails (it still has "display: none;")

  await fireEvent.click(openMenuButton)

  expect(menuItem).not.toBeVisible()
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@afontcu so just tried the menu in a project I have locally, the browser behaviour is:

  1. Mounted v-menu does not appear in the document
  2. Users clicks activator. In my case a v-text-field. v-menu is inserted into the DOM
  3. User clicks activator again to close the menu. v-menu is still in the DOM but has display: none

So the edit to the test looks valid based on this behaviour.
Looks like the test before was reporting a false positive.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like the test before was reporting a false positive.

Yes! That's what I thought, too 🙌 . The snippet I posted above is currently failing, I'll try to dig a bit more and see if I can figure out why. Thought it could have something to with transitions, even though I tried with <v-menu :transition="false" /> but nothing changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried a lot of different versions of this test, then figured:

"I wonder how Vuetify are testing the menu component"

Turns out the definition of this "working" is that it renders with display: none even when clicked whilst testing with VTU.

There is a snapshot here with display: none https://github.com/vuetifyjs/vuetify/blob/9ac1158f857fd1f531f1177ba623602973f81b55/packages/vuetify/src/components/VMenu/__tests__/__snapshots__/VMenu.spec.ts.snap#L15
With the corresponding test here https://github.com/vuetifyjs/vuetify/blob/9ac1158f857fd1f531f1177ba623602973f81b55/packages/vuetify/src/components/VMenu/__tests__/VMenu.spec.ts#L32

Copy link
Member

Choose a reason for hiding this comment

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

okay, this is interesting. Now I'm leaning towards simplifying our Vuetify test (which is only aimed at showing a way of configuring VTL and Vuetify) to make it more resilient. If they are happy with display:none, we should be, too. I'm gonna push some changes soon to this branch, and make the test pass.

Thanks for your help, @afenton90! Much appreciated :)

})
15 changes: 10 additions & 5 deletions src/vue-testing-library.js
View file
Open in desktop
@@ -1,4 +1,4 @@
import {createLocalVue, mount} from '@vue/test-utils'
import {createLocalVue, mount, config} from '@vue/test-utils'

import {
getQueriesForElement,
Expand All @@ -9,6 +9,10 @@ import {

const mountedWrappers = new Set()

// Added to remove warnings about usage of isVueInstance which is
// still used internally by @vue/test-utils
config.showDeprecationWarnings = false

function render(
TestComponent,
{
Expand All @@ -24,6 +28,9 @@ function render(
const baseElement = customBaseElement || customContainer || document.body
const container = customContainer || baseElement.appendChild(div)

const attachTo = document.createElement('div')
container.appendChild(attachTo)

const localVue = createLocalVue()
let vuexStore = null
let router = null
Expand Down Expand Up @@ -57,7 +64,7 @@ function render(
localVue,
router,
store: vuexStore,
attachToDocument: true,
attachTo,
sync: false,
...mountOptions,
...additionalOptions,
Expand Down Expand Up @@ -95,9 +102,7 @@ function cleanupAtWrapper(wrapper) {
document.body.removeChild(wrapper.element.parentNode)
}

if (wrapper.isVueInstance()) {
wrapper.destroy()
}
wrapper.destroy()

mountedWrappers.delete(wrapper)
}
Expand Down