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 full-content tests for parsing + blocks list + serialization #849

Merged
merged 11 commits into from
May 25, 2017

Conversation

nylen
Copy link
Member

@nylen nylen commented May 20, 2017

This PR adds a mechanism for testing all of the parsing and serialization logic at once:

  1. Start with full HTML content
  2. Parse it and verify the resulting blocks and their attributes against a JSON object
  3. Serialize the blocks list back to HTML and verify against a second chunk of HTML content

f1c0511 adds the code that makes this work, including some helper code that generates (2) and (3) from the list above given (1) as follows:

GENERATE_MISSING_FIXTURES=y npm run test-unit -- --grep 'full post content fixture'

Then, 7f30894 (very large commit - I would recommend reviewing only the first commit for now) adds a couple of test fixtures based on the demo post-content.js to demonstrate how this works. There are a couple of issues:

  • These are probably too large - I will prune them down + split out into more meaningful test cases later.
  • This doesn't actually work on Travis. It does on my computer, but Travis seems to be generating the key values of React elements differently. (Probably the way to solve this will be to do something more sophisticated than calling JSON.stringify on a React tree, see this thread for example.)

I had to make a couple of changes to the webpack config for this to work:

  • Enable node.__dirname so that the compiled test/build.js can read fixture files as expected.
  • Run the new full-content.js tests first because many of the other tests unregister all registered blocks (example).

See #745, though I am reluctant to close that one before we have a large number of these test cases in place.

@nylen nylen added [Feature] Blocks Overall functionality of blocks [Feature] Parsing Related to efforts to improving the parsing of a string of data and converting it into a different f [Status] In Progress Tracking issues with work in progress labels May 20, 2017
@nylen nylen requested review from youknowriad and aduth May 20, 2017 03:26
@youknowriad
Copy link
Contributor

I have a bad experience with tests like this (giant chunks of fixtures, with auto-generation). In my experience, this kind of tests breaks too often with false positives, because we continuously change the shape of the blocks or the output. And thus, we end up, regenerating the fixtures without taking much attention. I'd prefer several targeted tests instead of testing whole posts like this.

@nylen
Copy link
Member Author

nylen commented May 22, 2017

I think this kind of test is crucially important, though I agree we need to find a good balance between "easy to add a new test" and "lots of unnecessary noise". Here are some things I'd like to achieve here:

  • Test content in between / outside of blocks (this is currently pretty badly broken)
  • Test automatic parsing of blocks from content outside of blocks (initial PR on this coming soon)
  • Test at least one example for each block type that verifies parsing+serialization of all the block's attributes

I'd prefer several targeted tests instead of testing whole posts like this.

I agree, these need to be smaller. However, I am also hoping that later on when our internal block structure is more stable, we can use this test structure to fix + verify examples of problematic post content that we find during testing. Invalid markup inside blocks, invalid block types, etc.

we continuously change the shape of the blocks or the output

We shouldn't be changing the shape of the output so much anymore, so I'm pretty happy with the way steps (1) and (3) are working. For (2) I agree we need something much simpler; as long as we cover the list of blocks and their parsed attributes, this should be ok. Maybe the .json fixture would look more like this:

[
    {
        blockType: 'core/text',
        content: [
            '<p>some text</p>',
            '<p>second paragraph</p>'
        ]
    },
    ...
]

@youknowriad
Copy link
Contributor

Test at least one example for each block type that verifies parsing+serialization of all the block's attributes.

That's one of the reasons I put this PR in the first place #448 to allow each block test its own attributes (parsing included).


I'm not saying no to this PR, I wanted to share my concerns. And maybe, we should add more targeted tests to the parser unit tests to limit the scope of these fixtures tests.

@nylen nylen force-pushed the add/full-content-tests branch from 3f48cb9 to 3fc694a Compare May 23, 2017 01:27
@nylen
Copy link
Member Author

nylen commented May 23, 2017

I've updated the PR with smaller fixtures extracted from individual blocks in the demo content file. Maybe the core/table fixture in particular is still too big though fixed in 4333b90.

I've also simplified the way React trees are represented in the fixtures - core-text-multi-paragraph.json is a good example.

That's one of the reasons I put this PR in the first place #448 to allow each block test its own attributes (parsing included).

It still seems better to me to keep the blocks code simple and have the test code work around it. For now that means any code that tests our actual blocks needs to run first (before the tests that unregister all blocks), though we could definitely look at other approaches here.

And maybe, we should add more targeted tests to the parser unit tests to limit the scope of these fixtures tests.

What do you have in mind here? This is basically what this PR is doing now, with the advantage that it is very easy to add tests for new blocks because they are all done the same way.

@nylen nylen removed the [Status] In Progress Tracking issues with work in progress label May 23, 2017
@youknowriad
Copy link
Contributor

Actually, I'm way more satisfied with the current approach. It's way easier to reason block by block like this.

I wonder if we can't search for blocks and error (or warning) if a block is not tested like this. This would make this test more discoverable and we'll enforce block implementers to add this test each time a block is created.

Only applies to nested elements, not to blocks themselves.
@nylen nylen force-pushed the add/full-content-tests branch from 64fd1a5 to db47f0f Compare May 23, 2017 13:17
@nylen nylen force-pushed the add/full-content-tests branch from db47f0f to df253a2 Compare May 23, 2017 13:46
@nylen
Copy link
Member Author

nylen commented May 23, 2017

Actually, I'm way more satisfied with the current approach. It's way easier to reason block by block like this.

I agree. I would still like to add tests here for how we deal with content in between blocks, but this can wait until a future PR.

I wonder if we can't search for blocks and error (or warning) if a block is not tested like this. This would make this test more discoverable and we'll enforce block implementers to add this test each time a block is created.

Good idea. Done in df253a2 (and added missing fixtures for 2 blocks in 9f412a2).

@nylen nylen force-pushed the add/full-content-tests branch from 575b173 to 54a11f0 Compare May 23, 2017 15:47
@nylen
Copy link
Member Author

nylen commented May 23, 2017

Any objections to me merging this in the next day or so?

@nylen nylen merged commit a228bc0 into master May 25, 2017
@nylen nylen deleted the add/full-content-tests branch May 25, 2017 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks [Feature] Parsing Related to efforts to improving the parsing of a string of data and converting it into a different f
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants