Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hosting Command Palette: Add test cases for filtering logic #85838

Merged
merged 5 commits into from
Jan 10, 2024
Merged

Hosting Command Palette: Add test cases for filtering logic #85838

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b7b73ad
Add basic test cases for filtering logic
Dec 28, 2023
c0fe30e
Merge to trunk
Dec 29, 2023
4aacf5d
Add more cases for filtering
Dec 29, 2023
c79fcc9
Add tests case for no match
Dec 29, 2023
80c3009
Update to trunk
Jan 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions client/components/command-palette/test/use-command-filter.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
import { COMMAND_SEPARATOR, useCommandFilter } from '../use-command-filter';

describe( 'useCommandFilter', () => {
const commandFilter = useCommandFilter();

test( 'should return 1 for the exact match before separator', () => {
const search = 'Manage';
const beforeSeparator = 'Manage posts';
const afterSeparator = 'your blog posts';
const value = beforeSeparator + COMMAND_SEPARATOR + afterSeparator;
expect( commandFilter( value, search ) ).toBe( 1 );
} );

test( 'should return 1 for the exact match despite the case (lower or upper) before separator', () => {
const search = 'MANAGE';
const beforeSeparator = 'Manage posts';
const afterSeparator = 'your blog posts';
const value = beforeSeparator + COMMAND_SEPARATOR + afterSeparator;
expect( commandFilter( value, search ) ).toBe( 1 );
} );

test( 'should return 1 for the exact match with multiple words before separator', () => {
const search = 'manage posts';
const beforeSeparator = 'Manage posts';
const afterSeparator = 'your blog posts';
const value = beforeSeparator + COMMAND_SEPARATOR + afterSeparator;
expect( commandFilter( value, search ) ).toBe( 1 );
} );

test( 'should return 0.5 for the exact match after separator', () => {
const search = 'cache';
const beforeSeparator = 'Open hosting configuration';
const afterSeparator = 'cache hosting manage';
const value = beforeSeparator + COMMAND_SEPARATOR + afterSeparator;
expect( commandFilter( value, search ) ).toBe( 0.5 );
} );

test( 'should return 0.5 for the exact match after separator despite upper or lower case', () => {
const search = 'CACHE';
const beforeSeparator = 'Open hosting configuration';
const afterSeparator = 'cache hosting manage';
const value = beforeSeparator + COMMAND_SEPARATOR + afterSeparator;
expect( commandFilter( value, search ) ).toBe( 0.5 );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sejas I added test for this and the function for the filter does return 0.5 as expected so technically it does what the prop describes (return the number between 0 and 1) but it does not change the ranking. I think this indicates that there must be an issue with cmdk. What are your thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to add, I think we should also some tests to check the rendering of the commands when you are rending an actual array since these are fairly simple and only check if the matching works as expected

Copy link
Member

Choose a reason for hiding this comment

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

@sejas I added test for this and the function for the filter does return 0.5 as expected so technically it does what the prop describes (return the number between 0 and 1) but it does not change the ranking. I think this indicates that there must be an issue with cmdk. What are your thoughts?

I think we should report the issue to cmdk to fix it. I think they didn't create a release almost a year ago, so it could be they implemented it and didn't release it yet.

Copy link
Member

Choose a reason for hiding this comment

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

Just to add, I think we should also some tests to check the rendering of the commands when you are rending an actual array since these are fairly simple and only check if the matching works as expected

Yes! That would be fantastic. If it's not too complicated, we could create a fake array of commands, and then search for something and check if it's in the array or/and if it's a first result.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I will report the issue to them and work on the tests today! Thanks for the review!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sejas I search the cmdk repo and found the issue with filtering was reported here: pacocoursey/cmdk#118

There is current a PR in progress to fix the issue: pacocoursey/cmdk#182

} );

test( 'should return 0 where there is no match either for label or searchLabel', () => {
const search = 'CACHE';
const beforeSeparator = 'Open hosting configuration';
const afterSeparator = 'hosting manage';
const value = beforeSeparator + COMMAND_SEPARATOR + afterSeparator;
expect( commandFilter( value, search ) ).toBe( 0 );
} );
} );
Loading