-
Notifications
You must be signed in to change notification settings - Fork 273
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
remove Buffer from Mock*Service #1440
Conversation
This comment has been minimized.
This comment has been minimized.
the services based on tower-test were isolated in a task, which means that any panic due to mockall would stay in that task and not affect the rest of the tests. This rewrites the mock service system to provide clonable mock services that can panic
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!
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.
LGTM overall,
The only thing i m not comfortable with is the need for our plugin devs to call .expect_clone()
.
I think I'd even rather implement copy to our mocks to avoid having to do that.
WDYT is there a way we can get rid of this call?
it's not nice but AFAIK there is no way to just clone the expectations. And we can have different requirements depending on which instance of the service is called. I'd be open to a fix for that, but not in this PR: here I mainly want to have tests that work as expected |
Fix #1435
Note:
this does not fix this issue yet, because the service is isolated in a task, so any panic generated by mockall will stay in the task:it is fixed now:router/apollo-router/src/plugin/test/service.rs
Lines 26 to 35 in b55a628