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.

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 the block transforms. #12336

Merged

Conversation

jorgefilipecosta
Copy link
Member

@jorgefilipecosta jorgefilipecosta commented Nov 26, 2018

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.

@jorgefilipecosta jorgefilipecosta force-pushed the add/generic-end-2-end-test-block-transforms branch 2 times, most recently from c86eed0 to b7c0a55 Compare November 27, 2018 21:40
@jorgefilipecosta jorgefilipecosta requested a review from a team November 28, 2018 14:57
@gziolo gziolo added the [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. label Jan 31, 2019
gziolo
gziolo previously requested changes Jan 31, 2019
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

This PR needs to be refreshed since tests were moved to the new folder. I should have time next week to perform proper review of this PR if it updated :)

@gziolo gziolo added this to the 5.2 (Gutenberg) milestone Feb 7, 2019
@jorgefilipecosta jorgefilipecosta added the [Status] Blocked Used to indicate that a current effort isn't able to move forward label Feb 13, 2019
@gziolo gziolo removed this from the 5.2 (Gutenberg) milestone Feb 18, 2019
@gziolo
Copy link
Member

gziolo commented Feb 24, 2019

Is it still blocked given that we merged #13658 a few days ago?

@jorgefilipecosta
Copy link
Member Author

Hi @gziolo,
This PR was updated taking into account the last changes and it ready for a new review.

@jorgefilipecosta jorgefilipecosta removed the [Status] Blocked Used to indicate that a current effort isn't able to move forward label Feb 24, 2019
@gziolo gziolo added this to the 5.3 (Gutenberg) milestone Feb 25, 2019
@gziolo gziolo dismissed their stale review February 25, 2019 07:35

I'm on vacation this week and I don't want to block anyone else from reviewing and merging this one :)

@gziolo gziolo added the [Feature] Block Transforms Block transforms from one block to another label Feb 25, 2019
@jorgefilipecosta jorgefilipecosta force-pushed the add/generic-end-2-end-test-block-transforms branch 2 times, most recently from 7021600 to ea59dfc Compare March 20, 2019 09:53
@jorgefilipecosta
Copy link
Member Author

This PR is rebased and updated I think it is ready for the last checkup before the merge.

@@ -0,0 +1,183 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`test transforms individual transforms work as expected Code block should transform to Preformatted block. fixture: core__code 1`] = `
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't read very well. Ideally, it should be a readable sentence. E.g. RichText should insert character.

Copy link
Member

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 remove or reduce test transforms individual transforms work as expected ?

await page.click( '.editor-post-title .editor-post-title__block' );
} );

for ( const [ fixture, { originalBlock, availableTransforms } ] of Object.entries( EXPECTED_TRANSFORMS ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

There is describe.each API method, which you might also want to use here: https://jestjs.io/docs/en/api#describeeachtable-name-fn-timeout

It wouldn't have to be expressed using all those loops.

*
* @return {Promise} Promise resolving with a string containing the block title.
*/
export async function getBlockTitle( blockName ) {
Copy link
Member

Choose a reason for hiding this comment

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

How about making it more generic: getBlockSetting( blockName, settingName )?

Another idea, what if we create a wrapper over data API? It might not scale to keep creating helpers each time we want to get some data from the store.

@jorgefilipecosta jorgefilipecosta force-pushed the add/generic-end-2-end-test-block-transforms branch from ea59dfc to e5f6171 Compare March 22, 2019 15:38
@jorgefilipecosta jorgefilipecosta force-pushed the add/generic-end-2-end-test-block-transforms branch from e5f6171 to 79d5a5f Compare March 22, 2019 15:46
@jorgefilipecosta jorgefilipecosta merged commit f4de109 into master Mar 22, 2019
@jorgefilipecosta jorgefilipecosta deleted the add/generic-end-2-end-test-block-transforms branch March 22, 2019 16:29
@jorgefilipecosta
Copy link
Member Author

Thank you for the reviews @gziolo and @ellatrix, I applied the feedback before the merge.

)
);

it.each( testTable )(
Copy link
Member

Choose a reason for hiding this comment

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

Awesome, looks great 👍

@gziolo
Copy link
Member

gziolo commented Mar 25, 2019

I'm seeing the following error after rebasing my PR:

FAIL specs/block-transforms.test.js (27.737s)
  ● Block transforms › should contain the expected transforms
    expect(jest.fn()).not.toHaveErrored(expected)
    Expected mock function not to be called but it was called with:
    ["Block validation: Block validation failed for `%s` (%o).
    Content generated by `save` function:
    %s
    Content retrieved from post body:
    %s core/shortcode JSHandle@object  [gallery ids=\"238,338\"]"]
      at expect (../jest-console/build/@wordpress/jest-console/src/index.js:34:4)
  ● Block transforms › should contain the expected transforms
    expect(jest.fn()).not.toHaveWarned(expected)
    Expected mock function not to be called but it was called with:
    ["Block validation: Expected end of content, instead saw %o. JSHandle@object"]
      at expect (../jest-console/build/@wordpress/jest-console/src/index.js:34:4)

https://travis-ci.com/WordPress/gutenberg/jobs/187395326#L1050

@gziolo
Copy link
Member

gziolo commented Mar 25, 2019

I think I know what has happened :)
The issue is that we still don't override PHP file of dynamic blocks and the change I introduced doesn't load attributes of Shortcode block properly. You can skip it :)

@jorgefilipecosta
Copy link
Member Author

I think I know what has happened :)
The issue is that we still don't override PHP file of dynamic blocks and the change I introduced doesn't load attributes of Shortcode block properly. You can skip it :)

I guess this was the first problem this tests caught, it is a good sign :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block Transforms Block transforms from one block to another [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants