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
XDG stable: use mock methods instead of notification lists #275
Conversation
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.
Yeah, I broadly think this is a much better approach.
Please remove the catch-all expectations, and I'll be most pleased to land this 😁
// XdgSurfaceStable | ||
|
||
wlcs::XdgSurfaceStable::XdgSurfaceStable(wlcs::Client& client, wlcs::Surface& surface) | ||
{ | ||
EXPECT_CALL(*this, configure).Times(AnyNumber()); |
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 don't think we should add catch-all expectations like this to the generic code; it might make tests with specific expectations more difficult to write later.
Instead I think we should just instantiate NiceMock<XdgSurfaceStable>
s wherever we want to suppress the “No expectation matched” warnings.
{ | ||
state = wlcs::XdgToplevelStable::State{width, height, states}; | ||
state = wlcs::XdgToplevelStable::State{args...}; |
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.
Nice use of generic lambdas :)
{ | ||
should_close = true; | ||
}); | ||
EXPECT_CALL(xdg_toplevel, close()).Times(0); |
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'm not sure that this does what you want? GMock expectations are additive; how does this EXPECT_CALL().Times(0)
interact with the previous EXPECT_CALL().Times(AnyNumber())
?
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 believe new more specific EXPECT_CALL
's supersede old more general ones. If you remove the EXPECT_CALL(xdg_toplevel, close()).Times(1);
a few lines down the test fails.
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.
Fair enough.
bors r+
Simplifies and standardizes how events are handled in XDG types