Skip to content

Testing: Automate memoized selector setup clear#4938

Merged
aduth merged 1 commit intomasterfrom
update/selectors-clear-all
Feb 8, 2018
Merged

Testing: Automate memoized selector setup clear#4938
aduth merged 1 commit intomasterfrom
update/selectors-clear-all

Conversation

@aduth
Copy link
Copy Markdown
Member

@aduth aduth commented Feb 7, 2018

This pull request seeks to improve selector tests to automate memoized selector cache clearing. Gutenberg includes memoized selectors via rememo whose cache can taint the result of subsequent tests if not cleared during test lifecycle. Previously, this relied on the developer implementing caching to a selector to include their selector's clear call in the selector tests' beforeEach method. With these changes, this has been automated to remove manual developer intervention, detecting which selectors are cached, and automatically clearing them.

Testing instructions:

Verify that unit tests pass:

npm run test-unit

@aduth aduth added the [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. label Feb 7, 2018
useOnce: true,
} );

cachedSelectors = filter( selectors, property( 'clear' ) );
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically you could put it out of beforeAll and make it const.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, if I'm being honest, I'm not sure what difference is had between putting something in beforeAll vs. the top-level describe scope, so I was inclined to keep this as part of the lifecycle of the tests actually being run (assuming that's a valid distinction).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, it's not clear. When describe block is processed it executes all code in the scope and schedules all those special functions for future. So this is undocumented beforeBeforeAll step :) I put more things in the top-level describe these days. One use case when you would always use beforeAll is when you need to finish an async task.

Copy link
Copy Markdown
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea to trigger cache reset for every selector after each test. I also like how you filter selectors which are memoized 👍

@aduth aduth merged commit a451ffb into master Feb 8, 2018
@aduth aduth deleted the update/selectors-clear-all branch February 8, 2018 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants