refactor: MockHttpRuntime into PocketIcRuntime
#43
Conversation
lpahlavi
changed the base branch from
main
to
lpahlavi/add-readme-and-examples
lpahlavi
force-pushed
the
lpahlavi/add-live-mode-runtime
branch
4 times, most recently
from
4527c68 to
b494162
Compare
lpahlavi
force-pushed
the
lpahlavi/add-live-mode-runtime
branch
from
b494162 to
6195d06
Compare
gregorydemay
left a comment
gregorydemay
left a comment
There was a problem hiding this comment.
Some understanding questions
| /// Add a new mocked HTTP outcall. | ||
| pub fn push(&mut self, mock: MockHttpOutcall) { | ||
| self.0.push(mock); | ||
| pub fn push(&self, mock: MockHttpOutcall) { |
There was a problem hiding this comment.
understanding question: why is this no longer &mut self?
There was a problem hiding this comment.
Good question. I moved the Mutex directly to the MockHttpOutcalls struct so that I could implement the new ExecuteHttpOutcallMocks trait directly on it (i.e. we previously had a Mutex<MockHttpOutcalls(Vec<MockHttpOutcall>)>, now we have a MockHttpOutcalls(Mutex<Vec<MockHttpOutcall>>)).
This means all the operations on MockHttpOutcalls now use interior mutability and so don't need mut anymore. This is not ideal, but is a hack so that we can mutate mocks inside Runtime::update_call which takes an &self.
| self | ||
| } | ||
|
|
||
| /// Use Pocket IC in [live mode](https://github.com/dfinity/ic/blob/f0c82237ae16745ac54dd3838b3f91ce32a6bc52/packages/pocket-ic/HOWTO.md?plain=1#L43). |
There was a problem hiding this comment.
This might be a bit misleading because it doesn't change the PocketIC instance. Maybe ask Martin if there is a way to know whether a given PocketIc instance has live mode activated?
There was a problem hiding this comment.
Yes, good point. I had a look and couldn't find any method that tells whether a PocketIC instance is in live more... Martin is currently away but I'll have another look and worst case add a big fat warning.
| #[async_trait] | ||
| pub trait ExecuteHttpOutcallMocks: Send + Sync { | ||
| /// Execute HTTP outcall mocks. | ||
| async fn execute_http_outcall_mocks(&self, runtime: &PocketIc) -> (); |
There was a problem hiding this comment.
Unless I'm missing something, this will force anyone using that crate to have an async runtime when mocking HTTPs outcalls, even if their tests is actually not async. I think that's something we should ideally avoid (async should be opt-in).
There was a problem hiding this comment.
Hm, do you mean if someone wants to use the mocks without actually using Runtime? Runtime is an async trait (and has to be) so to me it's not a problem. Or did I not understand correctly?
lpahlavi
left a comment
lpahlavi
left a comment
There was a problem hiding this comment.
Thank you for having a look @gregorydemay! I've added some answers to your questions.
| /// Add a new mocked HTTP outcall. | ||
| pub fn push(&mut self, mock: MockHttpOutcall) { | ||
| self.0.push(mock); | ||
| pub fn push(&self, mock: MockHttpOutcall) { |
There was a problem hiding this comment.
Good question. I moved the Mutex directly to the MockHttpOutcalls struct so that I could implement the new ExecuteHttpOutcallMocks trait directly on it (i.e. we previously had a Mutex<MockHttpOutcalls(Vec<MockHttpOutcall>)>, now we have a MockHttpOutcalls(Mutex<Vec<MockHttpOutcall>>)).
This means all the operations on MockHttpOutcalls now use interior mutability and so don't need mut anymore. This is not ideal, but is a hack so that we can mutate mocks inside Runtime::update_call which takes an &self.
| self | ||
| } | ||
|
|
||
| /// Use Pocket IC in [live mode](https://github.com/dfinity/ic/blob/f0c82237ae16745ac54dd3838b3f91ce32a6bc52/packages/pocket-ic/HOWTO.md?plain=1#L43). |
There was a problem hiding this comment.
Yes, good point. I had a look and couldn't find any method that tells whether a PocketIC instance is in live more... Martin is currently away but I'll have another look and worst case add a big fat warning.
| #[async_trait] | ||
| pub trait ExecuteHttpOutcallMocks: Send + Sync { | ||
| /// Execute HTTP outcall mocks. | ||
| async fn execute_http_outcall_mocks(&self, runtime: &PocketIc) -> (); |
There was a problem hiding this comment.
Hm, do you mean if someone wants to use the mocks without actually using Runtime? Runtime is an async trait (and has to be) so to me it's not a problem. Or did I not understand correctly?
2 participants
This PR refactors the
MockHttpRuntimeinto the more genericPocketItRuntime, namely:PocketIcRuntimeto be used with or without HTTP outcall mocks.PocketIcRuntimeto be used in live mode.