From 028b6959c26f493ab4c4f2b90bd1b5fd6b94ab7e Mon Sep 17 00:00:00 2001 From: David Palm Date: Thu, 25 Jun 2020 16:14:02 +0200 Subject: [PATCH 1/7] Fix mocking multiple http calls in the same function call Fixes an issue where a function call would perform more than one http request and wait for each to complete before proceeding. The `RequestId` comes from the length of the `requests` collection in the `OffchainState` and if a request is completed before the next one starts it will be removed and the "next expected" will be off by one. This PR tries to fix that by using a request counter that tracks how many requests have been performed so that we can `remove()` items from the `expected_requests` at the right index. I suspect that this is a sub-optimal soluton and perhaps requests and their mocks should live side by side in the same collection, e.g. in a tuple of `(PendingRequest, Option)`. --- primitives/core/src/offchain/testing.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/primitives/core/src/offchain/testing.rs b/primitives/core/src/offchain/testing.rs index a14e906f54308..46e2d9f5801ea 100644 --- a/primitives/core/src/offchain/testing.rs +++ b/primitives/core/src/offchain/testing.rs @@ -120,6 +120,8 @@ impl OffchainStorage for TestPersistentOffchainDB { pub struct OffchainState { /// A list of pending requests. pub requests: BTreeMap, + /// Request counter + pub request_ctr: u16, expected_requests: BTreeMap, /// Persistent local storage pub persistent_storage: TestPersistentOffchainDB, @@ -156,10 +158,12 @@ impl OffchainState { } fn fulfill_expected(&mut self, id: u16) { - if let Some(mut req) = self.expected_requests.remove(&RequestId(id)) { - let response = req.response.take().expect("Response checked while added."); + if let Some(mut req) = self.expected_requests.remove(&RequestId(self.request_ctr)) { + let response = req.response.take().expect("Response checked when added."); let headers = std::mem::take(&mut req.response_headers); self.fulfill_pending_request(id, req, response, headers); + + self.request_ctr = self.request_ctr.saturating_add(1); } } From 35e2c959a0040d6809092a563fabdc0dbe36ed19 Mon Sep 17 00:00:00 2001 From: David Date: Thu, 25 Jun 2020 16:53:18 +0200 Subject: [PATCH 2/7] Update primitives/core/src/offchain/testing.rs Co-authored-by: Bernhard Schuster --- primitives/core/src/offchain/testing.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/primitives/core/src/offchain/testing.rs b/primitives/core/src/offchain/testing.rs index 46e2d9f5801ea..341ba06ce7398 100644 --- a/primitives/core/src/offchain/testing.rs +++ b/primitives/core/src/offchain/testing.rs @@ -158,7 +158,7 @@ impl OffchainState { } fn fulfill_expected(&mut self, id: u16) { - if let Some(mut req) = self.expected_requests.remove(&RequestId(self.request_ctr)) { + if let Some(mut req) = self.expected_requests.remove(&RequestId(self.request_counter)) { let response = req.response.take().expect("Response checked when added."); let headers = std::mem::take(&mut req.response_headers); self.fulfill_pending_request(id, req, response, headers); From 056ecc53501f19a368767df0b749c3cbb7c46880 Mon Sep 17 00:00:00 2001 From: David Date: Thu, 25 Jun 2020 16:53:24 +0200 Subject: [PATCH 3/7] Update primitives/core/src/offchain/testing.rs Co-authored-by: Bernhard Schuster --- primitives/core/src/offchain/testing.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/primitives/core/src/offchain/testing.rs b/primitives/core/src/offchain/testing.rs index 341ba06ce7398..02183313b3a02 100644 --- a/primitives/core/src/offchain/testing.rs +++ b/primitives/core/src/offchain/testing.rs @@ -121,7 +121,7 @@ pub struct OffchainState { /// A list of pending requests. pub requests: BTreeMap, /// Request counter - pub request_ctr: u16, + pub request_counter: u16, expected_requests: BTreeMap, /// Persistent local storage pub persistent_storage: TestPersistentOffchainDB, From b986bf4a76b4cb912a083904e10dc49d4984b3ae Mon Sep 17 00:00:00 2001 From: David Palm Date: Thu, 25 Jun 2020 17:03:03 +0200 Subject: [PATCH 4/7] Panic on overflow --- primitives/core/src/offchain/testing.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/primitives/core/src/offchain/testing.rs b/primitives/core/src/offchain/testing.rs index 02183313b3a02..cba315e438bb8 100644 --- a/primitives/core/src/offchain/testing.rs +++ b/primitives/core/src/offchain/testing.rs @@ -163,7 +163,7 @@ impl OffchainState { let headers = std::mem::take(&mut req.response_headers); self.fulfill_pending_request(id, req, response, headers); - self.request_ctr = self.request_ctr.saturating_add(1); + self.request_counter = self.request_counter.checked_add(1).expect("The max number of mocked requests is u16::MAX"); } } From 057604dd8e5b86a5bb96e5a7514d9db02ef0e31b Mon Sep 17 00:00:00 2001 From: David Date: Fri, 26 Jun 2020 00:15:50 +0200 Subject: [PATCH 5/7] Update primitives/core/src/offchain/testing.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- primitives/core/src/offchain/testing.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/primitives/core/src/offchain/testing.rs b/primitives/core/src/offchain/testing.rs index cba315e438bb8..4d92416c5df5f 100644 --- a/primitives/core/src/offchain/testing.rs +++ b/primitives/core/src/offchain/testing.rs @@ -163,7 +163,7 @@ impl OffchainState { let headers = std::mem::take(&mut req.response_headers); self.fulfill_pending_request(id, req, response, headers); - self.request_counter = self.request_counter.checked_add(1).expect("The max number of mocked requests is u16::MAX"); + self.request_counter += 1; } } From aebde5e803466b5de2593957be24b2b97aee5a10 Mon Sep 17 00:00:00 2001 From: David Palm Date: Fri, 26 Jun 2020 18:59:36 +0200 Subject: [PATCH 6/7] Use a Deque and push/pop expected requests --- frame/example-offchain-worker/src/tests.rs | 48 +++++++++++++++++++++- primitives/core/src/offchain/testing.rs | 16 ++++---- 2 files changed, 54 insertions(+), 10 deletions(-) diff --git a/frame/example-offchain-worker/src/tests.rs b/frame/example-offchain-worker/src/tests.rs index ef910b95ff5b9..b300809f4107b 100644 --- a/frame/example-offchain-worker/src/tests.rs +++ b/frame/example-offchain-worker/src/tests.rs @@ -154,6 +154,52 @@ fn should_make_http_call_and_parse_result() { }); } +#[test] +fn knows_how_to_mock_several_http_calls() { + let (offchain, state) = testing::TestOffchainExt::new(); + let mut t = sp_io::TestExternalities::default(); + t.register_extension(OffchainExt::new(offchain)); + + { + let mut state = state.write(); + state.expect_request(testing::PendingRequest { + method: "GET".into(), + uri: "https://min-api.cryptocompare.com/data/price?fsym=BTC&tsyms=USD".into(), + response: Some(br#"{"USD": 1}"#.to_vec()), + sent: true, + ..Default::default() + }); + + state.expect_request(testing::PendingRequest { + method: "GET".into(), + uri: "https://min-api.cryptocompare.com/data/price?fsym=BTC&tsyms=USD".into(), + response: Some(br#"{"USD": 2}"#.to_vec()), + sent: true, + ..Default::default() + }); + + state.expect_request(testing::PendingRequest { + method: "GET".into(), + uri: "https://min-api.cryptocompare.com/data/price?fsym=BTC&tsyms=USD".into(), + response: Some(br#"{"USD": 3}"#.to_vec()), + sent: true, + ..Default::default() + }); + } + + + t.execute_with(|| { + let price1 = Example::fetch_price().unwrap(); + let price2 = Example::fetch_price().unwrap(); + let price3 = Example::fetch_price().unwrap(); + + assert_eq!(price1, 100); + assert_eq!(price2, 200); + assert_eq!(price3, 300); + }) + +} + #[test] fn should_submit_signed_transaction_on_chain() { const PHRASE: &str = "news slush supreme milk chapter athlete soap sausage put clutch what kitten"; @@ -319,7 +365,7 @@ fn should_submit_raw_unsigned_transaction_on_chain() { } fn price_oracle_response(state: &mut testing::OffchainState) { - state.expect_request(0, testing::PendingRequest { + state.expect_request(testing::PendingRequest { method: "GET".into(), uri: "https://min-api.cryptocompare.com/data/price?fsym=BTC&tsyms=USD".into(), response: Some(br#"{"USD": 155.23}"#.to_vec()), diff --git a/primitives/core/src/offchain/testing.rs b/primitives/core/src/offchain/testing.rs index cba315e438bb8..9145477722d78 100644 --- a/primitives/core/src/offchain/testing.rs +++ b/primitives/core/src/offchain/testing.rs @@ -21,7 +21,7 @@ //! the extra APIs. use std::{ - collections::BTreeMap, + collections::{BTreeMap, VecDeque}, sync::Arc, }; use crate::offchain::{ @@ -120,9 +120,8 @@ impl OffchainStorage for TestPersistentOffchainDB { pub struct OffchainState { /// A list of pending requests. pub requests: BTreeMap, - /// Request counter - pub request_counter: u16, - expected_requests: BTreeMap, + // Queue of requests that the test is expected to perform (in order). + expected_requests: VecDeque, /// Persistent local storage pub persistent_storage: TestPersistentOffchainDB, /// Local storage @@ -158,12 +157,10 @@ impl OffchainState { } fn fulfill_expected(&mut self, id: u16) { - if let Some(mut req) = self.expected_requests.remove(&RequestId(self.request_counter)) { + if let Some(mut req) = self.expected_requests.pop_back() { let response = req.response.take().expect("Response checked when added."); let headers = std::mem::take(&mut req.response_headers); self.fulfill_pending_request(id, req, response, headers); - - self.request_counter = self.request_counter.checked_add(1).expect("The max number of mocked requests is u16::MAX"); } } @@ -173,11 +170,12 @@ impl OffchainState { /// before running the actual code that utilizes them (for instance before calling into runtime). /// Expected request has to be fulfilled before this struct is dropped, /// the `response` and `response_headers` fields will be used to return results to the callers. - pub fn expect_request(&mut self, id: u16, expected: PendingRequest) { + /// Requests are expected to be performed in the insertion order. + pub fn expect_request(&mut self, expected: PendingRequest) { if expected.response.is_none() { panic!("Expected request needs to have a response."); } - self.expected_requests.insert(RequestId(id), expected); + self.expected_requests.push_front(expected); } } From bcf5f0167c9415a5cadb79be4bcba1d1d7d26a7e Mon Sep 17 00:00:00 2001 From: David Palm Date: Fri, 26 Jun 2020 19:52:43 +0200 Subject: [PATCH 7/7] fix test --- client/executor/src/integration_tests/mod.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/client/executor/src/integration_tests/mod.rs b/client/executor/src/integration_tests/mod.rs index f07e98178b5d9..21924270b8c1e 100644 --- a/client/executor/src/integration_tests/mod.rs +++ b/client/executor/src/integration_tests/mod.rs @@ -497,9 +497,7 @@ fn offchain_http_should_work(wasm_method: WasmExecutionMethod) { let mut ext = TestExternalities::default(); let (offchain, state) = testing::TestOffchainExt::new(); ext.register_extension(OffchainExt::new(offchain)); - state.write().expect_request( - 0, - testing::PendingRequest { + state.write().expect_request(testing::PendingRequest { method: "POST".into(), uri: "http://localhost:12345".into(), body: vec![1, 2, 3, 4],