test: Add Writing Flow tests #49352
test: Add Writing Flow tests #49352
Conversation
dcalhoun
added
[Type] Automated Testing
|
Size Change: 0 B Total Size: 1.34 MB ℹ️ View Unchanged
|
dcalhoun
force-pushed
the
test/add-writing-flow-tests
branch
2 times, most recently
from
daddb63
to
f8d228b
Compare
|
Flaky tests detected in 762d3f3. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4547015371
|
dcalhoun
force-pushed
the
test/add-writing-flow-tests
branch
2 times, most recently
from
0cd36e3
to
e8196cc
Compare
The "Preformatted" description feels more appropriate and mirrors other block tests. I am uncertain as to why this was named differently originally.
Our helpers do not support typing multiple lines of text into Rich Text inputs. Ideally, we could add this feature to improve our test coverage.
This allows placing integration tests that target high-level features that span across multiple areas of the product, e.g. editor undo/redo history.
The previous matcher caused fixture files to be run as tests. Also, splitting the regex likely improves the comprehensibility of the expression.
There was a problem hiding this comment.
@fluiddot what do you think of this file placement and approach? There are subjects that are difficult to colocate with a unit of code, i.e editor history applies to various blocks.
If we like this approach, we could also hoist some of the text formatting tests set up for Paragraph to this location and run them against various Rich Text blocks, e.g. Heading, Quote.
There was a problem hiding this comment.
Definitely, it's tricky to find the right place for some of the test cases, especially if they are not tied to a component or block. This location looks good to me, in fact, replicates the integration folder we have for the web version 👍 .
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import PreformattedEdit from '../edit'; | ||
|
|
||
| describe( 'core/more/edit/native', () => { |
There was a problem hiding this comment.
Even after reviewing the project history, I am not sure where the core/more/edit/native name originated. It seemed incorrect and misleading. "Preformatted" felt clearer.
| keyCode: ENTER, | ||
| } ); | ||
|
|
||
| // TODO: Determine a way to type after pressing ENTER within the block. |
There was a problem hiding this comment.
Similar comments can be found through this PR. The changeTextOfRichText and changeAndSelectTextOfRichText helpers do not yet support typing multiple times into the same Rich Text input. Attempts to do so will clear out previously "typed" text.
I explored adding this support, but decided to consider it out of scope of this PR as it was taking too long to implement. I explored tracking previous text entries in local scope, further mocking Aztec implementations, and subscribing to store events. Unfortunately, I did not arrive to a working solution. More research is needed.
@dcalhoun @derekblank I understand that we'll be able to spot most of the regressions with these tests, but I'd like to note that part of the logic of the writing flow relies on the Aztec native library, which won't be covered with integration tests. For covering Aztec we'd need to do it via E2E tests. Do you foresee this caveat would impact the confidence in refactoring existing code? |
@fluiddot good question. I think we should consider this question in two contexts: refactoring to replace Aztec and refactoring for performance optimizations. For the former, I perceive the question will largely become moot as Aztec would no longer be a requirement. It would, in theory, be replaced by React Native's For the latter, your question is very relevant. By relying upon integration tests that mock Aztec, we expose ourselves to regressions that relate to Aztec itself or communication with it. I imagine Aztec itself is/should be covered with its own tests, which means we only need to decide if we want to test our communication with Aztec. This communication is likely pretty important and I could understand an argument we should test it. With these integration tests, I decided to balance speed, maintainability, coverage, and long-term goals. I believe they will provide value for one day replacing Aztec. IMO, this is the biggest and most necessary "win." I believe they provide some value for performance optimizations we choose to make. I imagine we will still perform manual tests for any performance optimizations we make, both due to the lack of Aztec coverage you reference and the fact that testing/verifying performance optimizations often benefit from experiencing the changes in "real world" scenarios. I will also note that we do plan to add e2e tests for aspects that are not easy or possible to test with integration tests, e.g. keyboard swiping. All that said, my thought was that integration tests for the writing flow made sense. However, I am open to approaching this in a different way with e2e tests. WDYT? |
Good point. I agree that we should rely as much as we can on integration tests. The rest of the cases that can't be done with integration tests could be left for the E2E tests, like for example, any test that needs visual testing. In this regard, I'm wondering how many of the writing flow tests would need it 🤔.
I understand that Aztec already has its tests to cover the logic. However, as we have a custom implementation of Aztec for Gutenberg, I don't think that part is actually covered. Eventually, if we consider adding tests for this area, I wouldn't add tests to cover all the functionality but focus on the most critical parts like the communication with the editor as you mentioned.
Makes total sense, as I mentioned earlier, it would be great to rely on integration tests as much as we can due to all the benefits they provide. In terms of performance, I understand that we can verify parts related to React like re-renders or render time with integration tests. But if we'd like to cover all performance areas, we still would need to have E2E tests (probably in the host apps) to measure performance as close to the user experience.
That's great, thanks for sharing 🙇 !
@dcalhoun These integration tests make total sense to me. My comment was just along the lines of making us aware that we won't fully cover all the logic related to the writing flow. I'd advocate following this approach and covering the native parts with future E2E tests for the most brittle areas like the communication with Aztec 👍. |
The test case spreadsheet represents my assessment of which type of test each test case needs. We, of course, can always reassess that, and I welcome other weighing in on that subject. Inline comments on the spreadsheet is likely the most productive form of discussion.
Good point.
Agreed. It is worth noting that running automated e2e performance tests would likely require leveraging a physical device farm to achieve semi-realistic results, i.e. I am not sure performance tests run on emulators would provide much value.
@fluiddot thank you for sharing your perspective. Much appreciated. |
Great, I'll take a look and share feedback 👍 . |
These were automated in WordPress/gutenberg#49352.
These were automated in WordPress/gutenberg#49352.
These were automated in WordPress/gutenberg#49352.
These were automated in WordPress/gutenberg#49352.
These were automated in WordPress/gutenberg#49352.
These were automated in WordPress/gutenberg#49352.
These were automated in WordPress/gutenberg#49352. This test was left in place due to the known issues, but there is little value in doing so. The known issues are capture as individual issues in GitHub. https://github.com/wordpress-mobile/test-cases/pull/147/files#r1255412693
* docs: Remove duplicative Dark Mode Group block test case This test case is duplicative of the Group test case: https://github.com/wordpress-mobile/test-cases/blob/2389290ec634b25dcd19d1d2af3badcf8591ff4a/test-cases/gutenberg/group.md#tc009 * docs: Remove automated Rich Text cases These were automated in WordPress/gutenberg#49352. * docs: Remove automated List splitting and merging test This was automated in WordPress/gutenberg#45048. * docs: Remove automated undo and redo test cases These were automated in WordPress/gutenberg#49352.
This test case was automated in WordPress/gutenberg#49352. The remaining known issues are captures in GitHub Issues.
This test case was automated in WordPress/gutenberg#49352. The remaining known issues are captures in GitHub Issues.
This test case was automated in: - WordPress/gutenberg#49352 - WordPress/gutenberg#53075 The remaining known issues are captures in GitHub Issues.
* docs: Remove automated multi-line component test case This test case was automated in: - WordPress/gutenberg#49352 - WordPress/gutenberg#53075 The remaining known issues are captures in GitHub Issues. * docs: Remove broken undo redo test link This test was removed in 316b1b0.
What?
Add automated test coverage for crucial writing flow interactions within the
native mobile editor.
Why?
Automated test coverage of these important features will enable us to avoid
introducing regressions while we add new features and refactor existing code to
optimize performance.
How?
Leverage existing integration test helpers to cover portions of the mobile app
writing flow with automated tests.
Testing Instructions
n/a
Testing Instructions for Keyboard
n/a
Screenshots or screencast
n/a