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

Tests: Update full post content tests to use snapshots #1841

Closed
wants to merge 1 commit into from

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Jul 10, 2017

This PR updates full post content tests to use snapshots. Diff is quite big, but changes were quite trivial:

  • Readme file got updated, might require some editorial work
  • I created 4 set of tests to keep assertions in their own methods
  • 3 tests verify logic using snapshots
  • I removed all no longer needed fixtures that aren't the initial post contents.

Testing

We need to verify if snapshots were properly created. From what I checked, they should be good.

Execute: npm run test-unit and make sure it still works.

Test Suites: 1 skipped, 31 passed, 31 of 32 total
Tests: 37 skipped, 571 passed, 608 total
Snapshots: 162 passed, 162 total
Time: 3.321s
Ran all test suites.

real 0m4.486s
user 0m22.232s
sys 0m1.611s

@gziolo gziolo added [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Status] In Progress Tracking issues with work in progress labels Jul 10, 2017
@gziolo gziolo self-assigned this Jul 10, 2017
@gziolo gziolo force-pushed the update/full-content-snaphots branch from 6705c8d to 8a1096c Compare July 10, 2017 16:53
@@ -93,110 +81,51 @@ describe( 'full post content fixture', () => {
require( 'blocks' );
} );

it( 'has 54 fixtures', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

It might be obsolete, but we can give it a try. Whenever we add/remove file, we would have to update this number.

} );
} );

it( 'should be present for each block', () => {
const errors = [];
const blockTypes = getBlockTypes();

expect( blockTypes ).toHaveLength( 51 );
Copy link
Member Author

Choose a reason for hiding this comment

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

Again, this check would help us control how many block types are processed. If we don't want to keep it updated whenever block type is added/removed, we should at least verify if this number is greater than 0.

@gziolo gziolo force-pushed the update/full-content-snaphots branch from 8a1096c to 09d8466 Compare July 11, 2017 06:12
@gziolo
Copy link
Member Author

gziolo commented Jul 11, 2017

It looks like phpunit tests depends on parsed fixtures:

There were 54 errors:

  1. Parsing_Test::test_parser_output with data set #0 ('core-embed__animoto.html', 'core-embed__animoto.parsed.json')
    Exception: Missing fixture file: '/home/travis/build/WordPress/gutenberg/blocks/test/fixtures/core-embed__animoto.parsed.json'

I'm wondering if we can read them from snaphshot...

@nylen
Copy link
Member

nylen commented Jul 24, 2017

I am pretty disappointed with Jest snapshots for this particular task. They break most of the design goals of this test suite:

  • Store fixture data in a way that is relatively easy to access and update (one clearly named set of files per test). Instead we end up with a single enormous snapshot file, which is all but guaranteed to be updated without close scrutiny of any changes.
  • Automatically share fixtures between client and server. It is very important to ensure identical parser behavior on the server and the client, but we would have to write a lot of custom logic to re-use the Jest snapshots in PHP. I don't think it is worth it.
  • Automatically verify that every registered block has at least one parsing/serialization test, with no updates required to the test code itself. Otherwise it is basically guaranteed that we will add blocks with zero tests.

The structure of this test suite (one file that processes all of these fixtures) is part of the problem. I'm fine with any solution or improvement here that decreases the complexity while preserving the above goals, but I don't think this is it. I'd rather see us using Jest snapshots for component testing as per their original purpose and as previously discussed at #1788.

@gziolo gziolo closed this Jul 26, 2017
@gziolo gziolo deleted the update/full-content-snaphots branch July 26, 2017 13:06
@gziolo
Copy link
Member Author

gziolo commented Jul 26, 2017

@nylen I must agree with you. If we didn't have to share all fixtures with PHP codebase this might work as expected, but given that Jest produces one snapshot file per test suite it doesn't make sense to promote usage of snapshots here.

Test file looks much cleaner with the changes applied in my opinion, so it might be a nice improvement if JS was our only concern here. Thanks for leaving your valuable feedback here. 👍

@gziolo gziolo removed the [Status] In Progress Tracking issues with work in progress label Jul 26, 2017
@nylen
Copy link
Member

nylen commented Jul 27, 2017

See #2018 for more discussion about using snapshots, this time for component testing.

Tug pushed a commit that referenced this pull request Feb 28, 2020
…edia_edit_icon_when_block_is_selected

[Media Edit Icon] Show the media edit icon only if the block is selected
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[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.

2 participants