Added integration tests for sending events to Zulip and mock server - #790
Added integration tests for sending events to Zulip and mock server#790cuba0001 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Hi, thank you, this looks good! One thing I'd like to modify is to remember the received messages "dynamically", so that we can check at the end of each test whether we got any expected messages or not. But for that we won't be able to use received_requests from wiremock anymore.
| ctx.get_next_comment_text(()).await?, | ||
| @"Tree closed for PRs with priority less than 5." | ||
| ); | ||
| assert_eq!( |
There was a problem hiding this comment.
It gets super annoying to update these test assertions when they change, which is why I prefer to use insta as much as possible.
Could you please turn this into something like:
let messages = ctx.zulip_messages().await?;
insta::assert_debug_snapshot(messages, @"");
| ctx.get_next_comment_text(()).await?, | ||
| @"Tree closed for PRs with priority less than 5." | ||
| ); | ||
| assert_eq!( |
There was a problem hiding this comment.
Same here. Could you please add a comment here that says that we want to ensure that the message gets @ stripped away? Thanks!
| ctx.get_next_comment_text(()).await?, | ||
| @"@default-user: :key: Insufficient privileges: not in review users" | ||
| ); | ||
| assert!(ctx.zulip_messages().await?.is_empty()); |
There was a problem hiding this comment.
Could you please add this to BorsTester::finish? To ensure that we didn't get any unexpected Zulip messages in tests. I think it could be added to assert_empty_queues.
To make this work, it would be good to change async fn zulip_messages to always take the received message, and flush it from the Zulip message queue. Otherwise we won't be able to recognize if a test read the message or not.
This comment has been minimized.
This comment has been minimized.
fd3cff8 to
0a1cb3c
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
Done! Added queue for received messages, added a check for receiving unexpected messages, also it required to add a check of messages count to the existing tests (which also flushes received messages), replaced assertions with insta, comment that says a message gets |
| let mock_server = MockServer::start().await; | ||
| let received_messages = Arc::new(Mutex::new(Vec::new())); | ||
|
|
||
| Mock::given(any()) |
There was a problem hiding this comment.
Could you please put back the explicit messages URL? It both helps the reader understand what is the test mocking, and it also ensures that we won't be mocking unexpected endpoints by accident.
| move |request: &Request| { | ||
| received_messages | ||
| .lock() | ||
| .push(ZulipMessage::try_from(request)); |
There was a problem hiding this comment.
Let's panic here if the message cannot be parsed, rather than putting results into the queue. The bors tester infrastructure should be able to detect panics within the mocked server.
| } | ||
|
|
||
| pub(super) fn take_received_messages(&self) -> anyhow::Result<Vec<ZulipMessage>> { | ||
| let received_messages: Vec<_> = self.received_messages.lock().drain(..).collect(); |
There was a problem hiding this comment.
(this code should be removed based on my previous suggestion, but here's a tip: you can collect a vec of results into a result of a vec)
| let received_messages: Vec<_> = self.received_messages.lock().drain(..).collect(); | |
| let received_messages: Vec<ZulipMessage> = self.received_messages.lock().drain(..).collect::<anyhow::Result<Vec<ZulipMessage>>()?; |
| Ok(messages) | ||
| } | ||
|
|
||
| pub(super) fn assert_empty_queue(&self) -> anyhow::Result<()> { |
There was a problem hiding this comment.
This should do the same thing as the GitHub mock, to stop the mocked server (the function will have to become async):
self.mock_server.reset().await;
drop(self.mock_server);3 participants
No description provided.