Core Data: Fix redo behavior and expand test coverage.

[epiqueras]

Fixes #17825

Description

This PR fixes an issue where explicit undo level creation was being ignored when the last recorded edit had no non-transient edits, i.e. not the blocks property. These explicit undo level actions were also creating an extra undo level, because of some conflicting logic with regular edit actions. This PR also fixes that.

How has this been tested?

E2E tests were expanded to verify that undoing and redoing creates and uses undo levels correctly.

Types of Changes

Bug Fix: Fix redo levels inconsistencies.

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.

[ellatrix]

Why is this timeout added? Can't we add a separate test case with the timeout? Are we still testing without the timeout if we add it here?

[ellatrix]

The test fail when this is removed, which is curious. This line shouldn't be necessary for it to pass.

[epiqueras]

We need it to make sure the timeout after typing for the undo level creation fires.

[ellatrix]

Hm, this shouldn't be necessary. You should be able to undo and redo without waiting a second.

[epiqueras]

How does the last level get created otherwise? It's implemented with a timeout.

[ellatrix]

Sure, but when you undo, and there is content in the undo buffer, the buffer should be undone first, then the undo level. Does that make sense?

[epiqueras]

Yes, but that's not how it currently works. I'll add that.

[ellatrix]

This is something that was previously tested with withHistory unit tests. Do you think it's possible to add unit tests for this new reducer?

[epiqueras]

Yes

[epiqueras]

ce90a82

[ellatrix]

I adjusted the test a bit so it also tests if the content is the same. It's still weird that the timeout at the start is needed now, but let's try to resolve that in a separate PR. This is already a vast improvement.

[epiqueras]

I adjusted the test a bit so it also tests if the content is the same.

We should bring back the snapshots I added. If we only compare the content against itself like that, we are not actually testing that the levels are being created at the right places, just that the total number of levels is correct. The snapshots actually tested that // Undo 2nd paragraph text., does exactly that and nothing else.

[ellatrix]

We should bring back the snapshots I added. If we only compare the content against itself like that, we are not actually testing that the levels are being created at the right places, just that the total number of levels is correct. The snapshots actually tested that // Undo 2nd paragraph text., does exactly that and nothing else.

It doesn't just test the total number of levels? It compares the content to make sure it's restored correctly. We take snapshots during the creation of the content and compare it to the content produced after undoing and redoing changes.

[epiqueras]

Yes, exactly, but it doesn't care about what that content is, so you're just testing the number of levels of content.

[ellatrix]

That's not just testing the number of levels? We don't care what the content is, we only care that the content is correctly undone and redone.

[epiqueras]

With the snapshots we also test that // Undo 2nd paragraph text. is actually undoing the 2nd paragraph text. With these comparisons, we only test that there are a total number of undos and redos and that the state before // Undo 2nd paragraph text. is equal to the state after // Redo 2nd paragraph text..

[ellatrix]

and that the state before // Undo 2nd paragraph text. is equal to the state after // Redo 2nd paragraph text..

So not just the number. 😉 It also tests that the state is equal to the one before any redo or undo actions occurred. We are saving the content while it's being created:

gutenberg/packages/e2e-tests/specs/undo.test.js

Lines 60 to 80 in 42167a2

	const firstBlock = await getEditedPostContent();
	await page.keyboard.type( 'This' );
	const firstText = await getEditedPostContent();
	await page.keyboard.press( 'Enter' );
	const secondBlock = await getEditedPostContent();
	await page.keyboard.type( 'is' );
	const secondText = await getEditedPostContent();
	await page.keyboard.press( 'Enter' );
	const thirdBlock = await getEditedPostContent();
	await page.keyboard.type( 'test' );
	const thirdText = await getEditedPostContent();

[epiqueras]

Yes, but that code won't detect if the behavior regresses from something like:

This(Undo Level)is(Undo Level)test

to

T(Undo Level)hisi(Undo Level)stest.

[ellatrix]

It would? It would not be the same as the content first created.

[epiqueras]

They would be the same, undo levels === redo levels, that's why I used snapshots.

[ellatrix]

Huh, we are not just comparing the amount of levels...

gutenberg/packages/e2e-tests/specs/undo.test.js

Line 98 in 42167a2

expect( await getEditedPostContent() ).toBe( firstText );

We are verifying if the content is the correct after undo/redo. If levels would be created in the wrong place, these checks would fail.

[ellatrix]

To me this is much clearer than snapshots, as we are explicitly comparing the initially created text with the changes after undo/redo.

[epiqueras]

Oh because the levels are created right after every .type. I see what you mean. Yeah this makes sense, sorry for the confusion.