Fix unit test for flaky filterSortAndPaginate function #70582
Merged
Conversation
This file contains hidden or 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: 0 B Total Size: 1.87 MB ℹ️ View Unchanged
|
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
I think @jsnajdr has some opinions about using fake timers :) |
Mamaduka
reviewed
Jul 1, 2025
Comment on lines
662
to
663
| { title: 'Recent', date: subDays( FIXED_DATE, 5 ) }, | ||
| { title: 'Old', date: subDays( FIXED_DATE, 14 ) }, |
There was a problem hiding this comment.
Have you tried creating a date instance at the test level and then reusing it to subtract days?
There was a problem hiding this comment.
Do you mean a format like this? It should probably work too.
it( '...', () => {
const FIXED_DATE = new Date( '2025-07-01T12:00:00.000Z' );
jest.useFakeTimers();
jest.setSystemTime( FIXED_DATE );
const testData = [
{ title: 'Recent', date: subDays( FIXED_DATE, 5 ) },
{ title: 'Old', date: subDays( FIXED_DATE, 14 ) },
];
// ...
} );However, unit tests are stable locally, so I won't know until I actually push it 😅
There was a problem hiding this comment.
Something like this, and avoid using fake timers.
it( '...', () => {
const FIXED_DATE = new Date( '2025-07-01T12:00:00.000Z' );
// Maybe even - const FIXED_DATE = new Date();
const testData = [
{ title: 'Recent', date: subDays( FIXED_DATE, 5 ) },
{ title: 'Old', date: subDays( FIXED_DATE, 14 ) },
];
// ...
} );However, unit tests are stable locally, so I won't know until I actually push it 😅
Same on my end.
There was a problem hiding this comment.
The following rarely fails locally:
it( '...', () => {
const FIXED_DATE = new Date();
const testData = [
{ title: 'Recent', date: subDays( FIXED_DATE, 7 ) },
{ title: 'Old', date: subDays( FIXED_DATE, 14 ) },
];
// ...
} );The following always fails locally:
it( '...', () => {
const FIXED_DATE = new Date( '2025-07-01T12:00:00.000Z' );
const testData = [
{ title: 'Recent', date: subDays( FIXED_DATE, 7 ) },
{ title: 'Old', date: subDays( FIXED_DATE, 14 ) },
];
// ...
} );The cause of the failure might be due to a mismatch with the new Date() executed in the filterSortAndPaginate() function.
|
I tried a completely different approach. I think the root problem is that the date or year we're subtracting in the test data is exactly the same as the number specified in the filter. From my understanding, the date/year used as the filter should be midway between the two test data. So I adjusted the condition so that the value I'm searching for in the filter is exactly halfway between the two test data. |
|
Thanks for working on stabilizing the test. My bad 😬 |
youknowriad
approved these changes
Jul 1, 2025
|
These tests passed at least three times, so it seems stable. Let's merge this PR. |
|
Flaky tests detected in 5abd985. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/16003364294
|
|
The test case feels a bit too loose right now—it doesn't really check that the operators are doing exactly what they're supposed to. It did help get rid of the flaky tests, which is great, but it also makes the test a bit less meaningful overall 😅 I proposed Automattic/wp-calypso#104549 to mock the result of |
Could you elaborate on this? I think the tests fixed in this PR seem to test basic comparison behavior rather than boundary value analysis. |
I don't get why you think like that. I think the changes make the tests better and more realistic. For example, consider the following:
const testData = [
{ title: 'Recent', date: subYears( new Date(), 1 ) },
{ title: 'Old', date: subYears( new Date(), 5 ) },
];
```ts
filters: [
{
field: 'date',
operator: 'inThePast',
value: { value: 3, unit: 'years' },
},
],expect( result ).toHaveLength( 1 );
expect( result ).toStrictEqual( [ testData[ 0 ] ] );The test is really clear that it will only get the Why do you think it's less meaningful? |
|
Yeah, the example above is meant to test "in the past 3 years", but the expected result only includes something from 1 year ago. How can we be sure that values from 2 years ago would still pass the test? If the goal is just to adjust the condition slightly, it might be better to shift it by a day or even a minute instead. Using a whole year feels like a really large range for this kind of test. Does it make sense? |
|
The goal of these unit tests is to make sure that the test data is filtered as expected according to the filter. I don't think the size of the range is essential here.
In unit testing, once you start thinking about things like this, there's no end to it. 😅 It is not possible to test every scenario. If you feel the current tests are insufficient, we can add new tests or increase the amount of test data. |
|
I think it's good enough because we know that the implementation is using basic comparison operator |
I don't think we should tie the test to the current implementation—if that changes and the test still passes, it may fail to catch real issues. What I’m trying to say is that we don’t necessarily need to test the boundary here. Instead, we can just slightly shift the original test date to account for the small timing differences caused by calling |
9 tasks
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?
Related to #70567
Trying to solve flaky unit tests for the
filterSortAndPaginate()function.Why?
The
new Date()is used inside thefilterSortAndPaginate()function and in unit tests. I suspected that this might be the cause of the instability.How?
In order to fix the time, introduce
useFakeTimersinto tests that are likely to be unstable.Testing Instructions
Let's run CI multiple times and see if the tests stabilize.