Refactor: Move block fixtures to e2e-tests package

[jorgefilipecosta]

Description

Required by: #12336

To have a generic end two end test that tests all block transforms, we need a way to access the block fixtures in the e2e test package.
This PR move the block fixtures into their own package so the fixtures can be accessed by both the full-content test and by the end two end tests.

How has this been tested?

I verified the unit tests pass.
I removed the /packages/blocks-fixtures/src/fixtures/core__audio* files and verified the unit tests start to fail because the audio block has no fixtures.
I removed the files (packages/blocks-fixtures/src/fixtures/core__audio.json,packages/blocks-fixtures/src/fixtures/core__audio.parsed.json,packages/blocks-fixtures/src/fixtures/core__audio.parsed.serialized.html). I executed the command GENERATE_MISSING_FIXTURES=y npm run test-unit test/integration/full-content/full-content.spec.jsand verified the fixtures for the audio block were generated.

[youknowriad]

My feedback here is that I'm not sure these fixtures tests are useful (or maybe not a lot) and they are a burden to maintain so I'm a bit concerned about the desire to make them more important and use them in other tests. Thoughts?

[jorgefilipecosta]

My feedback here is that I'm not sure these fixtures tests are useful (or maybe not a lot) and they are a burden to maintain so I'm a bit concerned about the desire to make them more important and use them in other tests. Thoughts?

I think one of the reasons they are not very useful at the moment is because the tests we do with them are relatively simple.
But the idea of forcing us to provide a fixture for each block is important. These fixtures can then be used for much more complex generic tests:

• The transforms test, adds each fixture verify it contains the expected transforms. Execute each transform make sure everything went as expected. (this simple test makes sure the transforms work and then all the blocks don't crash in the states caught by the fixtures).
• A generic rich-text test. Adds each fixture checks if there is an editable, apply some rich text operations and make sure the block worked as expected.
• Generic extensibility test. Register some predefined extensibility points, e.g: add a new attribute and show it on block edit, and on save ( a color picker for an icon). Adds each block and verify the block was extended as expected.

Having fixtures ensure that for each block we have at least one fixture that captures the most common state of the block. Tests using these fixtures should be made by us and by the community interacting with our blocks (e.g: plugin devs to make sure their plugins don't break default blocks).
Exposing these fixtures enables this set of new usages and may make them useful. Right now we have the fixtures and we had the work to create and maintain them but we did not use their full potential.

[youknowriad]

I understand that and often these "generic" tests are seen as a quicker way to get more coverage but I'm still skeptical:

• Did we find any regressions with these tests that were not captured by other tests?
• In my experience, generic tests are often a bad option to go for. I prefer writing manual tests, it takes more time but it ensures we don't update the fixtures blindly when they fail and we're testing something very tangible.

I might be biased because all my previous experiences with generic tests seemed like just loss of time for me. Happy to follow other opinions and see.

[jorgefilipecosta]

Hi @youknowriad, before going the general path I tried specific tests to the transforms (which currently have almost no tests at all), and I think having manual tests for transforms does not add value when compared with a generic solution and increases the test code to maintain. I found my self repeating the code for each transform.
E.g: if we want to test a specific type of transform with the generic version we just need to create a fixture with the input transform (which is trivial to do), and that's it no more code is needed. In a manual approach, we will for sure need more code, and I don't think it will test additional parts specific to the transform so we will probably have no gain.

• In my experience, generic tests are often a bad option to go for. I prefer writing manual tests, it takes more time but it ensures we don't update the fixtures blindly when they fail and we're testing something very tangible.

In the transforms case even if generic we are testing also something very tangible we have an input and an expected output if they are different we are sure the transform is not working as expected or it was changed and the tests need an update it is exactly the same situation as a manual test.
We have an enormous amount of possible transforms (https://github.com/WordPress/gutenberg/blob/d0718f2597a5f1b64ab39e53ad6b1862c058ae3e/test/e2e/specs/__snapshots__/block-transforms.test.js.snap generated snapshots for 55 of them), manually testing we will probably test 10% of them, so to make sure each transform function is executed at least once probable generic tests are a good option.

• Did we find any regressions with these tests that were not captured by other tests?

Until now the tests failed for two reasons which are related to us having invalid fixtures. Fixtures reference media URLs that trigger 404 errors, or the block in the fixture is are in an invalid state e.g.: referencing an external resource but also still containing an id attributing referencing a file of the media library. The snapshots generated need a manual inspection one by one the first time they are created to make sure the tests are valid, and I did make that process exhaustively yet.

I agree in that generic tests should not be too generic, but if they bulk test something very specific I think they may be beneficial.

Also, it is important to note that fixtures may also be useful in "manual" non-generic tests. Fixtures provide examples of the most common states a block can be in, and even in a manual test, we can leverage a fixture to make the creation of the test easier.

[youknowriad]

Also, it is important to note that fixtures may also be useful in "manual" non-generic tests. Fixtures provide examples of the most common states a block can be in, and even in a manual test, we can leverage a fixture to make the creation of the test easier.

This makes sense.

Another question, why an npm package? do we need these fixtures outside of the Gutenberg repository? Why not just a shared folder test/block-fixtures or something. An npm package has costs so I'm wondering if it's necessary.

[gziolo]

Another question, why an npm package? do we need these fixtures outside of the Gutenberg repository? Why not just a shared folder test/block-fixtures or something. An npm package has costs so I'm wondering if it's necessary.

There is another PR from @jorgefilipecosta where he starts using those fixtures also for e2e tests. That's why it makes sense to move them to packages. However, I also don't feel like it needs be a new one. It seems like it would be perfectly fine to include them inside packages/e2e-tests. It's not perfect but it would be one less package to maintain and you could still reference them from integration tests.

Another question is whether we should keep integration tests for those fixtures. As soon as we build e2e tests which consume those fixtures, I would argue that they are going to duplicate the same validations. In addition, those integration tests are disconnected from the real usage and I don't have enough confidence in what they generate.

[jorgefilipecosta]

Another question, why an npm package? do we need these fixtures outside of the Gutenberg repository? Why not just a shared folder test/block-fixtures or something.

@gziolo was correct is because we need the fixtures to be accessed by our e2e test package. And exposing the fixtures in the package also allows third-party plugins to use our fixtures in their tests.

@gziolo I followed your suggestion and now the fixtures are part of e2e test package.

[gziolo]

It looks like basename is a thing: https://en.wikipedia.org/wiki/Basename

It might be fine to leave it as it was. @aduth @nerrad or @adamsilverstein? What do you think?

[nerrad]

If you're asking whether basename should be left used instead of baseName I would agree, it does mean a number of other new code introduced in this pull that would need updated as well so that basename is used consistently.

In the same vein, filename should probably be used instead of fileName (https://en.wikipedia.org/wiki/Filename).

[gziolo]

English is hard when it meets computer science :)

[nerrad]

heh, English is hard period (even to native speakers) 😄

[gziolo]

@jorgefilipecosta thanks for starting the process of moving fixtures to e2e tests. I really appreciate your effort. As mentioned already, I think that we should simplify our setup by replacing all the tests in test/integration/full-content/full-content.spec.js with a corresponding set of e2e tests which represent a real-life usage rather than something that produces so much hassle with running fixture cleanup and generation.

I added two commits:

• d3cb985 - I changed the way fixtures are exposed, in fact they are no longer part of public API, you can still reference them outside by referencing fixtures folder directly
• fc9dc60 - I updated all occurences to filename and basename as discussed in the comment.

I also rebased PR with the latest changes from master.

When testing I noticed that fixtures:clean was no longer working which is fixed now.

I also noticed that fixtures:server-registered is broken, but this should be investigated separately. @aduth it's related to the PHP code removal as far as I can see. Ideally, we remove this logic and move all tests to e2e tests 🤷‍♂️

[jorgefilipecosta]

Hi @gziolo thank you for iterating on this, the changes look good to me 👍

[aduth]

I don't think it was a legitimate use-case to disable the rule here. If it's part of the package's public interface, it should be exposed from the root. We shouldn't ever need to path-traverse a module to import.

[gziolo]

Agreed in general, however as explained I did it on purpose to avoid exposing those methods at the moment. We will have to maintain them forever so let's test them with our own e2e tests first.

I also left this note:

I think that we should simplify our setup by replacing all the tests in test/integration/full-content/full-content.spec.js with a corresponding set of e2e tests which represent a real-life usage rather than something that produces so much hassle with running fixture cleanup and generation.

If we get rid of those integration tests we will also won't need to have those methods exposed as part of the package's public interface. There is another issue which you raised in #13583 and I think noticed in one of the similar PRs which adds a different new block. It seems like those tests are sometimes broken and don't generate the correct output which makes them only confusing. It's very hard to validate them if you don't know exactly how they should behave.

[aduth]

@aduth it's related to the PHP code removal as far as I can see. Ideally, we remove this logic and move all tests to e2e tests 🤷‍♂️

I missed this earlier. It's fixed with #14097