-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[WIP] Add a basic test for shortcode transformation #7689
Conversation
The significant underlying challenge, as documented in the diff, with adding tests around On a related note, it's annoying to need to redefine all of the block types in the test. I suspect this will be very brittle and break as soon as any block type definition changes. Related #6512 As a dirty hack to get things started, I've injected |
innerBlocks: [], | ||
isValid: true, | ||
name: 'core/shortcode', | ||
uuid: 'invalid', |
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.
uuid
being a random value is the cause of the test failure right now. What's the best way of addressing?
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.
Not entirely sure it works in this specific usage, but perhaps expect.any( String )
?
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.
Not entirely sure it works in this specific usage
Yeah, I suppose I could make the assertions a bit more verbose and inspect each part of the array.
Another idea is to create a wrapper for the uuid
function that lets us inject some state for the purposes of testing.
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.
expect.any( String )
is better, we don't care about what uuid
produces in this context.
Related: #336 |
@@ -58,16 +54,19 @@ function segmentHTMLToShortcodeBlock( HTML, lastIndex = 0 ) { | |||
( schema ) => schema.shortcode( match.shortcode.attrs, match ), | |||
); | |||
|
|||
blockType = find( blockTypes, { name: transformation.blockName } ); |
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.
Noting that getBlockType
is O(1)
(object lookup by property) and this is O(n)
@WordPress/gutenberg-core Suggestions on how to move forward with this? My preference is to not modify I'll need to resolve this PR in order to move forward with #7030, #4456 and others. |
Could we call |
Assuming you want to use existing blocks, that would be the easiest way to go. Otherwise, you can also mock |
627c886
to
bdd8da3
Compare
🎉 That worked way better than I expected. Thanks @noisysocks ! Closing in favor of #7823 |
The beginnings of the solution to #7030, #4456, etc.