-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[RFC] Add vitest integration for package unit testing #6089
base: develop
Are you sure you want to change the base?
Conversation
|
✅ Deploy Preview for tiptap-embed ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Should this only live on |
}, | ||
resolve: { | ||
alias: { | ||
'@tiptap/extension-bold': path.join(__dirname, 'packages/extension-bold/src'), |
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.
The alias resolution is needed so the Vitest
runner knows how to resolve the @tiptap/extension-bold
, @tiptap/extension-document
, and other import aliases correctly
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.
Seems verbose, I'd have thought there was a way to resolve using a tsconfig
@@ -0,0 +1,11 @@ | |||
<!DOCTYPE html> |
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.
Since we rely on JSOM
to mount an in-memory DOM
so we can bind the Editor
to an HTMLElement
this file is needed as it serves JSDOM
for the initial DOM
structure
|
||
describe('Bold Extension', () => { | ||
beforeAll(async () => { | ||
dom = await JSDOM.fromFile(path.resolve(__dirname, '..', 'test.dom.html'), { |
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.
JSDOM
initialization, maybe this can be extracted into a separate utility
extension => extension.name === 'bold', | ||
) | ||
|
||
expect(boldExtension).toBeTruthy() |
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 test for an extension's presence within the editor
extensions: [Document, Paragraph, Text, Bold], | ||
}) | ||
|
||
expect(editor.commands.setBold).toBeDefined() |
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 test that commands inserted by extensions are present in the editor
|
||
editor.commands.setBold() | ||
|
||
expect(editor.getText()).toContain('Example Text') |
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 is very naive, but we could get the specific node from the editor and check for it's textStyle
here, or their marks to check that the bold
mark is actually being applied
@tiptap/core
@tiptap/extension-blockquote
@tiptap/extension-bubble-menu
@tiptap/extension-bullet-list
@tiptap/extension-character-count
@tiptap/extension-bold
@tiptap/extension-code
@tiptap/extension-code-block
@tiptap/extension-code-block-lowlight
@tiptap/extension-collaboration
@tiptap/extension-collaboration-cursor
@tiptap/extension-color
@tiptap/extension-document
@tiptap/extension-dropcursor
@tiptap/extension-floating-menu
@tiptap/extension-focus
@tiptap/extension-font-family
@tiptap/extension-gapcursor
@tiptap/extension-hard-break
@tiptap/extension-heading
@tiptap/extension-highlight
@tiptap/extension-history
@tiptap/extension-horizontal-rule
@tiptap/extension-image
@tiptap/extension-italic
@tiptap/extension-link
@tiptap/extension-list-item
@tiptap/extension-list-keymap
@tiptap/extension-mention
@tiptap/extension-ordered-list
@tiptap/extension-paragraph
@tiptap/extension-placeholder
@tiptap/extension-strike
@tiptap/extension-subscript
@tiptap/extension-superscript
@tiptap/extension-table
@tiptap/extension-table-cell
@tiptap/extension-table-header
@tiptap/extension-table-row
@tiptap/extension-task-item
@tiptap/extension-task-list
@tiptap/extension-text
@tiptap/extension-text-align
@tiptap/extension-text-style
@tiptap/extension-typography
@tiptap/extension-underline
@tiptap/extension-youtube
@tiptap/html
@tiptap/pm
@tiptap/react
@tiptap/starter-kit
@tiptap/suggestion
@tiptap/vue-2
@tiptap/vue-3
commit: |
As this is more like the initial state of the Vitest setup I wanted to make sure that it works fine with one extension and now we could build up from there to have it in all extensions, either way, there's no need from |
…lready depends on build
"jsdom": "^26.0.0", | ||
"vitest": "^3.0.5" |
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'd not make this a dev dep of the package. It should only be for testing & having it in the root should be enough
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 PR should have been done on the next
branch since that will be the next major version. v2 should be in maintenance only mode right 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 not sure about this, the sorts of tests that you can assert here are not anything that cannot be exercised by cypress in an e2e fashion.
E2E sort of is the best approach for editor testing because it exercises actual paths people would interface with rather than just that commands exist or not
}, | ||
resolve: { | ||
alias: { | ||
'@tiptap/extension-bold': path.join(__dirname, 'packages/extension-bold/src'), |
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.
Seems verbose, I'd have thought there was a way to resolve using a tsconfig
@@ -14,6 +14,10 @@ | |||
"dev": { | |||
"persistent": true, | |||
"cache": false | |||
}, | |||
"test": { | |||
"dependsOn": ["^build"], |
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.
Does it depend on build? based on your path resolution it is using the source files & not the built files
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.
Sorry meant to reject
Note
RFC
We would like to discuss if there's actually a need to introduce Vitest to have another testing step along with Cypress or if with Cypress would be enough
This pull request introduces several changes to the project, primarily focusing on adding unit testing capabilities using Vitest, updating dependencies, and configuring the testing environment. The most important changes are summarized below:
Testing Enhancements:
package.json
andpackages/extension-bold/package.json
. [1] [2]vitest.config.ts
to set up the testing environment with JSDOM and alias resolutions.extension-bold
package to include JSDOM environment setup and alias resolutions.Dependency Updates:
jsdom
andvitest
as new dependencies inpackage.json
andpackages/extension-bold/package.json
. [1] [2] [3]New Tests:
bold.spec.ts
for theextension-bold
package to test the initialization and commands of the Bold extension.test.dom.html
to serve as the test document for the Bold extension tests.