-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Add generic end 2 end test to all block transforms #10536
Conversation
d76d1b1
to
a013ad4
Compare
@@ -7,7 +7,7 @@ | |||
</li> | |||
<li class="blocks-gallery-item"> | |||
<figure> | |||
<img src="http://google.com/hi.png" alt="title" /> | |||
<img src="https://cldup.com/cXyG__fTLN.jpg" alt="title" /> |
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 you explain this change? Thanks!
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.
Oh I see now, never mind. Would be cool if the image moved so it's in our control.
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.
Yes, that's true but what reliable source can we consider under our control?
I used cldup here as we are also using it on the demo page so at least we only depend on this domain.
@@ -1,3 +1,3 @@ | |||
<!-- wp:core/video --> | |||
<figure class="wp-block-video"><video controls src="https://awesome-fake.video/file.mp4"></video></figure> | |||
<figure class="wp-block-video"><video controls src="https://www.sample-videos.com/video/mp4/720/big_buck_bunny_720p_1mb.mp4"></video></figure> |
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.
Same here. We have no control over sample-videos.com
? What if something happens with that site?
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 will switch to a cldup url.
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 don't know about cldup either tbh. I think any files should be in this repo. Test images an files could be super tiny.
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 guess it can be cloud up for now as we're already doing that.
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.
Tiny files might speed up the tests as well though.
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.
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 like this idea, but I'm not certain how we can make sure that a local URL for a file is available. The other option would use a data URL that contains the files inline but for example, a video still contains many characters.
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.
Apparently it would be:
data:video/mp4;base64,AAAAHGZ0eXBpc29tAAACAGlzb21pc28ybXA0MQAAAAhmcmVlAAAAGm1kYXQAAAGzABAHAAABthBgUYI9t+8AAAMNbW9vdgAAAGxtdmhkAAAAAMXMvvrFzL76AAAD6AAAACoAAQAAAQAAAAAAAAAAAAAAAAEAAAAAAAAAAAAAAAAAAAABAAAAAAAAAAAAAAAAAABAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAgAAABhpb2RzAAAAABCAgIAHAE/////+/wAAAiF0cmFrAAAAXHRraGQAAAAPxcy++sXMvvoAAAABAAAAAAAAACoAAAAAAAAAAAAAAAAAAAAAAAEAAAAAAAAAAAAAAAAAAAABAAAAAAAAAAAAAAAAAABAAAAAAAgAAAAIAAAAAAG9bWRpYQAAACBtZGhkAAAAAMXMvvrFzL76AAAAGAAAAAEVxwAAAAAALWhkbHIAAAAAAAAAAHZpZGUAAAAAAAAAAAAAAABWaWRlb0hhbmRsZXIAAAABaG1pbmYAAAAUdm1oZAAAAAEAAAAAAAAAAAAAACRkaW5mAAAAHGRyZWYAAAAAAAAAAQAAAAx1cmwgAAAAAQAAAShzdGJsAAAAxHN0c2QAAAAAAAAAAQAAALRtcDR2AAAAAAAAAAEAAAAAAAAAAAAAAAAAAAAAAAgACABIAAAASAAAAAAAAAABAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAGP//AAAAXmVzZHMAAAAAA4CAgE0AAQAEgICAPyARAAAAAAMNQAAAAAAFgICALQAAAbABAAABtYkTAAABAAAAASAAxI2IAMUARAEUQwAAAbJMYXZjNTMuMzUuMAaAgIABAgAAABhzdHRzAAAAAAAAAAEAAAABAAAAAQAAABxzdHNjAAAAAAAAAAEAAAABAAAAAQAAAAEAAAAUc3RzegAAAAAAAAASAAAAAQAAABRzdGNvAAAAAAAAAAEAAAAsAAAAYHVkdGEAAABYbWV0YQAAAAAAAAAhaGRscgAAAAAAAAAAbWRpcmFwcGwAAAAAAAAAAAAAAAAraWxzdAAAACOpdG9vAAAAG2RhdGEAAAABAAAAAExhdmY1My4yMS4x
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 a small amount of characters, but it still might have a big impact on test speed and safeness.
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.
Image: data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///yH5BAEAAAAALAAAAAABAAEAAAIBRAA7
}; | ||
|
||
describe( 'test transforms', () => { | ||
const expectedTransforms = JSON.parse( |
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 put this in a .js
file?
@@ -0,0 +1,297 @@ | |||
// Jest Snapshot v1, https://goo.gl/fbAQLP | |||
|
|||
exports[`test transforms individual transforms work as expected Should correctly transform block in fixture core__audio to File block 1`] = ` |
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.
Is there any way to make this more descriptive and concise? Something like: "Paragraph block should transform to Heading block"?
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.
We can do that, but the thing is that we have many tests testing Paragraph to Heading block (one for each paragraph fixture), that was the reason I added the fixture to make it easier to debug in case the test fails. Maybe adding also the block name is a good trade-off.
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.
Ok, we can always iterate on this.
fileBase | ||
); | ||
} | ||
expect( console ).toHaveErroredWith( |
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.
Why are we expecting this? Could this be commented?
const transformStructure = {}; | ||
beforeAll( async () => { | ||
await newPost(); | ||
await page.click( '.editor-post-title .editor-post-title__block' ); |
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.
Why is this needed? Just wondering.
); | ||
} ); | ||
|
||
it( 'Should contain the expected transforms', async () => { |
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.
Generally I think we should not capitalise "should" because it will be placed in a sentence.
describe( 'individual transforms work as expected', () => { | ||
afterEach( async () => { | ||
await setPostContent( '' ); | ||
await page.click( '.editor-post-title .editor-post-title__block' ); |
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.
After reading beforeAll
, it should like this should happen beforeEach
?
"core__archives":[], | ||
"core__archives__showPostCounts":[], | ||
"core__audio":[ | ||
"File" |
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.
Any way to have this both be block names (plus attributes) or both be file names?
@@ -0,0 +1,32 @@ | |||
/** |
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.
Is there any of this that could be shared with full-content.spec.js
?
b5aaec7
to
7ffc1dc
Compare
Hi @iseulde, thank you a lot for your review, I think all the feedback was applied. |
2d6c12a
to
822a947
Compare
Squashed commits: [fd5c73823] Ignore missing block. [0d9ef5e70] Updated fixtures. [9d04f9909] Used utils function in full-content [321564b94] Improvements [fe4bd6fbc] Console errors fixed seems to be working without expecting any error. [a4f802ca1] Finalized implementation. [79a135b3d] Add end 2 end test block transforms.
822a947
to
d0718f2
Compare
Closed in favor of #12336. |
## Description This is a second try on PR #10536, after a recommendation by @gziolo to try to make the PR smaller so it becomes easier to review. In order to make the PR smaller, I temporarily created a list of test fixtures that intersects with the set of all fixtures so most fixtures are ignored. Another PR will follow that corrects the fixtures (mainly media ones) so they are reliable and can be used in this tests (e.g: don't trigger 404 requests). All the mechanisms are present in this PR the only change that will happen in the other PR is the removal of the intersection and its list, the update of some media fixtures and the creation of new snapshots. This is a high-level test that is going to Guarantee that every possible transform on Gutenberg is tested and works as expected (the result matches the save snapshot). ## How has this been tested? Verify all test cases pass.
Description
This PR proposes a generic approach to test the block transforms we have.
Fist it loads all the fixtures of content we have and checks if the transforms that should appear on the block they represent correctly appear.
Then it checks in a loop if each transform contains the output that should is expected.
If in the future a new block is created a new fixture should create for it. If that block contains transforms, we should update our fixtures/block-transforms.json specifying which transforms are available and a new test case is automatically generated and a snapshot created. if the fixtures/block-transforms.json file is not updated the tests will fail so this makes sure the tests do not get outdated, each transform is tested.