-
-
Notifications
You must be signed in to change notification settings - Fork 455
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 input completion test suite #4644
Conversation
I'll note that this PR does not include completion tests for username nor commands. Testing these would require a fully mockable This is one of the advantages of the new system in the pending PR as strategies are testable outside of sources. |
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.
clang-tidy made some suggestions
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.
Small things I noticed while looking at this.
I made the mock change to make them accessible for both tests & benchmarks here #4645 |
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.
Small nitpicks, haven't done a thorough review yet
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.
clang-tidy made some suggestions
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.
👍 Thank you!
Description
Adds tests for
InputCompletionPopup
andCompletionModel
behavior as a precursor to merging #4639. While developing these tests I identified two discrepancies in behavior from #4639 which I will fix (eventually).This PR needed to make some changes to
BttvEmotes
(among others) to allow directly setting loaded emotes for mocking. It looks a bit weird but it seems necessary.Benchmarking
I've all but finished writing benchmarks for the existing input completion code. However, I can't figure out a smart way to access
tests/src/mocks
from both tests and benchmarks. The code should obviously be shared. I tried moving it tosrc
but that would require linkinggtest
intochatterino-lib
(I think), which I don't understand the full implications of (nor would be I able to modify the cmake config to do so).If anyone (read: pajlada) can think of a way to share the mocks between tests and benchmarks and how to configure
CMakeLists.txt
(as I also can't figure that out when simply copyingtests/src/mocks
tobenchmarks/src/mocks
), I'd be very grateful :)