useEntityRecord: Do not trigger REST API requests when disabled #56108
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
Mamaduka
added
[Type] Regression
Mamaduka
requested review from
adamziel and
matiasbenedetto
and removed request for
nerrad
|
Size Change: -202 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
ntsekouras
reviewed
Nov 16, 2023
|
|
||
| // Fetch request should have been issued | ||
| await waitFor( () => | ||
| expect( triggerFetch ).not.toHaveBeenCalledWith( { |
There was a problem hiding this comment.
I think we should test what we expect and not something different. Probably use expect( triggerFetch ).not.toHaveBeenCalled(). The main thing though is that this test(even with my suggestion) passes in trunk without your changes. 🤔
There was a problem hiding this comment.
That's odd. This was failing without changes in my test. I'll have a loot at it tomorrow.
There was a problem hiding this comment.
I've updated the tests. They should fail on trunk but pass on this branch.
There was a problem hiding this comment.
This is really weird :). I also tried yesterday to reset the mocks etc.., and for me the issue remains, but I think it's something we miss with the testing library.
When I comment out your (!enabled) code and run the test, it still passes in trunk. When I'm changing though the second check to:
expect( triggerFetch ).toHaveBeenCalledWith( {
path: '/wp/v2/widgets/2?context=edit',
} )
(removing the not). The tests works as expected, but the expect( triggerFetch ).not.toHaveBeenCalled(); doesn't throw, which it should. I'll approve this PR now because the fix is good, but I'm curious why this is happening. I'll cc @kevin940726 and if we need to adjust something, let's do it in a follow up.
There was a problem hiding this comment.
I think apiFetch isn't dispatched when we do an assertion for the first time. But I wasn't able to figure out why. This seems to work for the first test.
cc @tyxla
There was a problem hiding this comment.
waitFor will immediately resolve if it passes the first time, so basically there's no waiting. This is tricky for testing something that "shouldn't" happen. Instead, I think we can fallback to the good old setTimeout and set an arbitrary reasonable waiting time.
await act(
() => new Promise( ( resolve ) => setTimeout( resolve, 0 ) )
);
expect( triggerFetch ).not.toHaveBeenCalled();In this context, setTimeout of 0 works, but it really depends on the implementation details. Awaiting a microtask (promise) doesn't work here, for instance.
If we want to make this test more resilient to implementation details, then we can first make a "enabled" assertion (.toHaveBeenCalled()) with the setTimeout(, 0) approach then make another "disabled" assertion with the same setTimeout(, 0) setup.
There was a problem hiding this comment.
waitFor will immediately resolve if it passes the first time, so basically there's no waiting.
It makes sense. Thank you!
There was a problem hiding this comment.
Sorry for coming late for this convo, but yes - what @kevin940726 said 👍
There was a problem hiding this comment.
If we want to make this test more resilient to implementation details, then we can first make a "enabled" assertion (.toHaveBeenCalled()) with the setTimeout(, 0) approach then make another "disabled" assertion with the same setTimeout(, 0) setup.
@kevin940726, can you elaborate bit more about this part? 🙇
There was a problem hiding this comment.
Sure! Because we want to make our unit tests as fast as possible, but we also don't want to just specify a random magic number (0) that could silently break when the implementation details change. Consider the below example:
const TestComponent = ({ enabled }) => {
data = useEntityRecord( 'root', 'widget', 2, { enabled } );
return <div />;
};
const UI = ({ enabled }) => (
<RegistryProvider value={ registry }>
<TestComponent enabled={enabled} />
</RegistryProvider>
);
// Smoke test.
const { rerender } = render(<UI enabled={true} />);
await act(() => new Promise(resolve => setTimeout(resolve, 0))); // The minimum delay.
expect(triggerFetch).toHaveBeenCalledTimes(1);
// Real test.
rerender(<UI enabled={false} />);
await act(() => new Promise(resolve => setTimeout(resolve, 0))); // The same delay.
expect(triggerFetch).toHaveBeenCalledTimes(1);If the minimum delay (implementation detail) is still shorter than setTimeout(,0) then every thing works correctly and the second assertion is enough to test the feature as shown in the table below.
| Assertion passes? | First assertion | Second assertion |
|---|---|---|
enabled:false works |
✅ | ✅ |
enabled:false breaks |
✅ | ❌ |
However, if the implementation detail changes to a longer delay (one animation frame for instance), the second assertion won't catch the regression and we'll need the first assertion (smoke test) to help us.
| Assertion passes? | First assertion | Second assertion |
|---|---|---|
enabled:false works |
❌ | ✅ |
enabled:false breaks |
❌ | ✅ |
I think I'm overly complicating this now 😅. In short, the first assertion is just a smoke test to help us make sure the wait is enough for the hook to call apiFetch (when it should).
kevin940726
reviewed
Nov 17, 2023
| let data; | ||
| const TestComponent = () => { | ||
| data = useEntityRecord( 'root', 'widget', 2, { | ||
| options: { enabled: false }, |
There was a problem hiding this comment.
This should just be:
{ enabled: false },
Passing any object still works because we're only defaulting enabled to true if options is undefined. Otherwise, options.enabled will always be falsy (undefined) unless explicitly set to true. I think this is another bug we should fix.
|
|
||
| // Fetch request should have been issued | ||
| await waitFor( () => | ||
| expect( triggerFetch ).not.toHaveBeenCalledWith( { |
There was a problem hiding this comment.
waitFor will immediately resolve if it passes the first time, so basically there's no waiting. This is tricky for testing something that "shouldn't" happen. Instead, I think we can fallback to the good old setTimeout and set an arbitrary reasonable waiting time.
await act(
() => new Promise( ( resolve ) => setTimeout( resolve, 0 ) )
);
expect( triggerFetch ).not.toHaveBeenCalled();In this context, setTimeout of 0 works, but it really depends on the implementation details. Awaiting a microtask (promise) doesn't work here, for instance.
If we want to make this test more resilient to implementation details, then we can first make a "enabled" assertion (.toHaveBeenCalled()) with the setTimeout(, 0) approach then make another "disabled" assertion with the same setTimeout(, 0) setup.
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?
A regression after #39595.
The
useEntityRecordhook shouldn't call resolves and trigger REST API requests whenoptions.enabledis set tofalse.Why?
PR fixes a regression.
Testing Instructions