Fix mocking multiple http calls in the same function call (#6510)

* 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<ExpectedRequest>)`.

* Update primitives/core/src/offchain/testing.rs

Co-authored-by: Bernhard Schuster <bernhard@ahoi.io>

* Update primitives/core/src/offchain/testing.rs

Co-authored-by: Bernhard Schuster <bernhard@ahoi.io>

* Panic on overflow

* Update primitives/core/src/offchain/testing.rs

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

* Use a Deque and push/pop expected requests

* fix test

Co-authored-by: Bernhard Schuster <bernhard@ahoi.io>
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

substrate/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],

substrate/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()),

substrate/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,7 +120,8 @@ impl OffchainStorage for TestPersistentOffchainDB {
 pub struct OffchainState {
 	/// A list of pending requests.
 	pub requests: BTreeMap<RequestId, PendingRequest>,
-	expected_requests: BTreeMap<RequestId, PendingRequest>,
+	// Queue of requests that the test is expected to perform (in order).
+	expected_requests: VecDeque<PendingRequest>,
 	/// Persistent local storage
 	pub persistent_storage: TestPersistentOffchainDB,
 	/// Local storage
@@ -156,8 +157,8 @@ 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.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);
 		}
@@ -169,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);
 	}
 }