Size Change: 0 B

Total Size: 1.23 MB

_{compressed-size-action}

but shouldn't we respect the existing steps when migrating tests?

It depends on the test. If the test is about inserting blocks and ensuring users could type right after inserting them, then yes, absolutely. However, if the test is about switching to the plain style of the quote block, which is what this test is about, then I think we're allowed to use some shortcuts.

The same applies to the purpose of those REST helpers to clear/set states between tests. Ideally, we would want to do what end users do in our tests, but often that's too slow and sometimes flaky. To prevent testing the same thing over and over again, we create some shortcuts/mocks to do them in a much faster way. It's covered in the best practices of the documentation.

Unless I misunderstood what this test is about, I think we should test the abovementioned behavior in another test, specifically for this kind of interaction. WDYT?

(Additionally, the new insertBlock behavior doesn't insert blocks from the inserter anymore, but rather inserts them from calling wp.data.dispatch('core/block-editor').insertBlock() in the console. Hence, the original behavior will still be lost even if we add back page.type after the insertion.)

Thanks for the explanation. Though I'm now a bit more confused 😅 Sorry if I'm missing context, I don't have tons of experience with the way e2e tests are set up. I guess my core question is: doesn't using different code paths than a user would defeats the purpose of the e2e tests? How e2e tests built this way are different than unit tests?

Unless I misunderstood what this test is about, I think we should test the abovementioned behavior in another test, specifically for this kind of interaction. WDYT?

I'm happy to add more specific tests, but for the kind of flow I wanted to test (focus, typing, etc, what we had before) I'd need to make sure that we're actually using the user code paths (the inserter, etc). Have looked around but don't find a way to do that, is there any? I guess I'd need to write a new utility that actually uses the inserter?

I agree with @kevin940726 on principle but I don't think a migration should not change how tests work unless we also cover the part that was changed.

(Additionally, the new insertBlock behavior doesn't insert blocks from the inserter anymore, but rather inserts them from calling wp.data.dispatch('core/block-editor').insertBlock() in the console. Hence, the original behavior will still be lost even if we add back page.type after the insertion.)

This for me is very problematic. insertBlock is used in so many tests and in so many different ways that if we decided to change its behavior, it's going to have an enormous impact on the coverage. Every time we migrate a test that uses it, we should be thinking about what we're losing in terms of coverage (and this PR is the perfect example).

I'd have preferred personally to avoid changing any behavior for the migration itself, and having dedicated PRs to change the behavior because that way we can think clearly about the impact and don't miss anything while migrating.

For this PR, it's probably better to just add a new test covering the typing but I'm really concerned about the "change everything approach" we seem to be taking with the migration.

doesn't using different code paths than a user would defeats the purpose of the e2e tests? How e2e tests built this way are different than unit tests?

It depends on what you want to test. I'm not good with words, so probably the documentation of Cypress and the referenced video there could explain it better 😅 .

I don't think a migration should not change how tests work unless we also cover the part that was changed.

Ideally, I totally agree with this. But in practice, this will probably slow down the migration process by a lot, and we'll not be using some of the benefits that Playwright brings until very late in the process.

Every time we migrate a test that uses it, we should be thinking about what we're losing in terms of coverage (and this PR is the perfect example).

IMO, code path coverage isn't what's important in e2e tests; rather, the coverage of what the test is about is more important. The migration process is never going to cover all existing coverage, there are always going to be some differences, and that's okay.

I'd have preferred personally to avoid changing any behavior for the migration itself, and having dedicated PRs to change the behavior because that way we can think clearly about the impact and don't miss anything while migrating.

This is an option, but the fact that so many tests depend on e2e-test-utils makes it complicated. The package depends on Puppeteer, and even though they are very much alike, there are still many fundamental differences that make them incompatible in many ways. We could migrate all utils from e2e-test-utils to their one-to-one mapping in e2e-test-utils-playwright as closely as possible. But the approach I took is to gradually rewrite part of the tests during the migration to follow the new best practices. The plan is to migrate tests to strictly follow their original testing purposes as much as possible, and safely rewrite those intermediate steps (activateTheme, insertBlock, etc) that are not relevant to the outcome of the tests.

IMO, we should focus on what we want to test, or else it's difficult for contributors to change any of the existing tests, and thus making the purpose of the tests blur and not readable to others. (Everything is important will make nothing important.)

As you suggested before, deleting tests is a best practice, we shouldn't be afraid of removing exiting coverage if that's not even the part of the test itself.

As you suggested before, deleting tests is a best practice, we shouldn't be afraid of removing exiting coverage if that's not even the part of the test itself.

Good one :)