Navigation: Add integration tests for NavigationMenuSelector component #47825
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +1.19 kB (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 710b682. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4122909054
|
getdave
force-pushed
the
try/nav-block-integration-test-experiment
branch
from
b82a195 to
0685b3d
Compare
getdave
commented
Feb 7, 2023
| @@ -93,10 +93,10 @@ function NavigationMenuSelector( { | |||
| hasResolvedNavigationMenus && currentMenuId === null; | |||
|
|
|||
| useEffect( () => { | |||
| if ( ! hasResolvedNavigationMenus ) { | |||
| if ( ! hasResolvedNavigationMenus && ! canUserCreateNavigationMenu ) { | |||
There was a problem hiding this comment.
Bug fix: should show option to create even if menu items have no resolved.
getdave
commented
Feb 7, 2023
| setSelectorLabel( __( 'Loading …' ) ); | ||
| } else if ( noMenuSelected || noBlockMenus || menuUnavailable ) { | ||
| setSelectorLabel( __( 'Choose a Navigation menu' ) ); | ||
| setSelectorLabel( __( 'Choose or create a Navigation menu' ) ); |
There was a problem hiding this comment.
Fix: sometimes the "Create" option can show even if there are no menus. We should make the label clearer.
getdave
commented
Feb 7, 2023
| @@ -56,7 +56,7 @@ function NavigationMenuSelector( { | |||
| return ( | |||
| navigationMenus?.map( ( { id, title }, index ) => { | |||
| const label = | |||
| decodeEntities( title.rendered ) || | |||
| decodeEntities( title?.rendered ) || | |||
There was a problem hiding this comment.
Bug fix: if there is no rendered property at all we need to handle that and ensure we get a fallback title.
getdave
force-pushed
the
try/nav-block-integration-test-experiment
branch
from
252b410 to
49b797e
Compare
getdave
changed the title
|
@glendaviesnz I've tagged you into this one as I know you are keen to explore integration testing. This is an example of such an approach. It's very simple but it provides confidence. |
|
I found a bug! If you click the block inserter to add a new block, and select a custom link, two new menus are created: It would be good to add a test for this! |
Seems broken on @scruffian How would you feel about merging this "as is" and then following up with a fix and a test. Reason being this PR is getting large. |
scruffian
approved these changes
Feb 8, 2023
|
Nice - we need a lot more of this style of test across the blocks - thanks for the ping. |
DaisyOlsen
added
[Package] Block library
DaisyOlsen
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Adds a suite of integration/component level tests for the
<NavigationMenuSelector>component in the Nav block.Also fixes a few bugs discovered along the way.
Why?
This component has suffered from a number of regressions. Moreover it has a number of states and potential edge cases that are too difficult for humans to manually test. This makes the component brittle and difficult to change.
Adding these tests provides an intial measure of confidence in the component's behaviour. After this it will be necessary to refactor the component to remove some of the unusual patterns that are in evidence (e.g. setState on render). This should help to simplify it but should not be done without the backing of tests.
How?
Adds integration tests that assert on all the states of the component. Various hooks are mocked to control the state of the component. That's ok. We sacrifice some certainty/accuracy for speed and ease of use. It's gets us 80% of the coverage with 20% of the effort relative to e2e tests.
Testing Instructions
It's critical we try and verify the original behaviour from
trunkis maintained:With that checked you can run the test suite:
Testing Instructions for Keyboard
Screenshots or screencast