-
Notifications
You must be signed in to change notification settings - Fork 9
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.
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
Add 2 scenarios for typeahead search #44
Conversation
* 1 for when search goes offline prior to search * 1 for a search query that shows results Fixes: T306846
Thanks for the PR! I think this will really help cut down on the search regressions as well as help with the codex switch |
Managed to work out the sticky header scenario.. |
// Wait for the loader to display | ||
await page.waitForSelector( '.search-form__loader', { | ||
visible: true | ||
} ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed this border animation was still flaking. I think adding the following line will stop the flakiness
// Fast forward any transitions.
await fastForwardAnimations( page );
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
configDesktop.js
Outdated
label: 'Test (#vector-2022, #search-focus, #search-offline)', | ||
path: '/wiki/Test', | ||
// account for mismatch due to border color change animation | ||
misMatchThreshold: 0.5, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure this is needed. Please see my comments in search.js.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
await moduleReady( page, 'vue' ); | ||
// focus and type into the newly added input | ||
await page.focus( 'input[name="search"]' ); | ||
await page.keyboard.type( 't' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of this one vs. only having page.keyboard.type( 'test' )
? I'm asking because I noticed the results for t
were captured in one test instead of the two results for test
and I'm wondering if this line is necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was seeing some flakiness where the first keystroke didn't show any results. Let's remove it and see if it still passes..
I was noticing the results for `tes` instead of `test` in some scenarios because of the typeahead search 120ms debounce. Using one letter instead of several should eliminate this.
Fixes: T306846
I tried to add one for the sticky header but it complicated things. I don't think we necessarily need to have one either, given logged in users account for less than 1% of our traffic.