-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feat: adding ui component tests #590
feat: adding ui component tests #590
Conversation
✅ Deploy Preview for vitest-dev ready! 🔨 Explore the source changes: 170f15a 🔍 Inspect the deploy log: https://app.netlify.com/sites/vitest-dev/deploys/62385ecb22b7670008899b9b 😎 Browse the preview: https://deploy-preview-590--vitest-dev.netlify.app |
Very nice work ! |
It is, but you'd need to create a common entry file instead. I think duplication is okay -- I find I don't edit this list too often. |
packages/ui/cypress/support/index.ts
Outdated
import 'codemirror/lib/codemirror.css' | ||
import 'codemirror-theme-vars/base.css' | ||
import 'tippy.js/dist/tippy.css' | ||
import '../../client/styles/main.css' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we share them somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can but you'd need to create like... another entry file. I can do it, I just never really need to in other projects. There aren't usually a lot of global deps.
@@ -0,0 +1,18 @@ | |||
import faker from '@faker-js/faker' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha, happy to see we use it! 🙌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AMAZING!
Co-authored-by: Michel EDIGHOFFER <edimitchel@gmail.com>
…essicaSachs/vitest into jess/adding-cy-component-tests
Test failed on CICD, should we work on birpc stubbing or just disable UI test on CICD for now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sure about testids but this could avoid some repetition label, no?
@@ -0,0 +1,30 @@ | |||
import TestsEntry from './TestsEntry.vue' | |||
|
|||
const passEntrySelector = '[data-testid=pass-entry]' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we share and export that testids from components?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've never seen a pattern like that in a codebase, nor have I implemented it before because:
- The indirection makes debugging why a certain element wasn't found more difficult.
Instead of one step: Cmd+F data-testid="pass-entry-selector"
You'd need to do two: Cmd+F pass-entry-selector
and then Cmd+F passEntrySelector
to find its usage within the template.
- The boilerplate required makes the impl less readable.
- DataTestID is only used some of the time for selectors.
[role="button"]
and other selector strategies are also valid, and you'd create an inconsistency
For the implementation, I guess you would do:
<script lang="ts">
export const passEntryDataTestId = 'pass-entry-selector'
</script>
<script setup lang="ts">
// ....
</script>
<template>
<div :data-testid="passEntryDataTestId" />
</template>
import Component, { passEntryDataTestId } from './Component.vue'
const passEntrySelector = `[data-testid=${passEntryDataTestId}]`
// test...
I would never recommend doing this. I hope this helps answer your question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... A bit of an off-topic aside, since you seem to be into the theory behind testing...
One of the patterns that isn't as common nowadays than it was for QA Engineers is the Page Object Model (POM) pattern. This pattern essentially has you build a Class representation of the "Pages" in your application. It was very common pre-2015 when writing Selenium tests.
// CheckoutForm.vue might have a POM that represents how to interact and get data on it.
class CheckoutForm {
constructor(products) {
products.forEach(() => addProductSomehow()) // either via UI or via some mock
}
products() {
return cy.get('[data-testid=product]')
}
product(idx = 0) {
return this.products.at(idx)
}
productRemoveButton(idx = 0) {
return this.product(idx).findByLabel('Remove')
}
removeProduct(idx = 0) {
return this.productRemoveButton(idx).click()
}
}
and a test...
it('can remove products and checkout', () => {
const products = [ /* two products */ ]
const checkoutForm = new CheckoutForm(products)
checkoutForm
.products()
.should('have.length', 2)
.deleteProduct()
.products()
.should('have.length', 1)
})
Cypress doesn't really have good patterns for PageObjectModel (We actually discourage users from doing it, though I've never really agreed with why.) I've considered exploring this pattern again from the perspective of developers instead of QA Engineers, but haven't done so.
The downsides are a superset of the downsides that come with OOP. The upsides are the same. It promises to offer composition through actions and data. For example a CheckoutPage
may have a CheckoutForm
and you can compose with classes.
Anyway, this is just an exercise in thought-leadership -- it's an uncommon and antiquated pattern at the moment and I wouldn't recommend it for production usage.
😅
I have no idea why the DashboardEntry file isn't building. It's only failing in CI, not locally... I think it might have to do with a race condition (the machines in CI are slower) and the prebundling necessary for the cjs modules we use in the UI. I could probably hack a workaround, but I think there's an issue with Cypress CT + unocss. I'm going to mark this PR as draft and open up an issue in Cypress and investigate. Super bummed. |
export const registerMount = () => Cypress.Commands.add( | ||
'mount', | ||
// eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
<C extends Parameters<typeof mount>[0]>(comp: any, options: any = {}) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This C generic seems unused
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah thanks for talking about this. I really need help learning how to best type these wrapper functions. Typescript is not my first language... 😓
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should give a try of https://github.com/type-challenges/type-challenges!
In a stream, this could be fun to watch, especially with Anthony as mentor :P
FYI, we're still working on this. A little delayed in doing it, but I'm unblocked by @patak-dev. |
Now that Patak released Vite 2.9.0-beta.3 I'll pick this back up and check |
CI is currently broken with typechecks on a bug in Vitest where it won't work in "global" mode with other test runners (The Chai assertions conflict). Locally it seems like the bump to Vite 2.9.0-beta.3 will fix the underlying stability issue. I could either:
I've never done No. 2 and don't know if it's possible to avoid global type conflicts like this. |
I didn't understand what |
Adding Cypress Component Testing. This is branched off of @edimitchel's #585.
The UI isn't very testable as-is. Not a big deal so longn as we get infrastructure in for mocking the rpc and ws connections.