Fix test

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

substrate/frame/message-queue/src/integration_test.rs

@@ -151,6 +151,7 @@ fn stress_test_recursive() {
 		TotalEnqueued::set(TotalEnqueued::get() + enqueued);
 		Enqueued::set(Enqueued::get() + enqueued);
 		Called::set(Called::get() + 1);
+		Ok(())
 	}));
 
 	build_and_execute::<Test>(|| {

substrate/frame/message-queue/src/mock.rs

@@ -164,7 +164,6 @@ impl ProcessMessage for RecordingMessageProcessor {
 		meter: &mut WeightMeter,
 		_id: &mut [u8; 32],
 	) -> Result<bool, ProcessMessageError> {
-		sp_io::storage::set(b"transactional_storage", &vec![1, 2, 3]);
 		processing_message(message, &origin)?;
 
 		let weight = if message.starts_with(&b"weight="[..]) {
@@ -185,7 +184,9 @@ impl ProcessMessage for RecordingMessageProcessor {
 		if meter.try_consume(required).is_ok() {
 			if let Some(p) = message.strip_prefix(&b"callback="[..]) {
 				let s = String::from_utf8(p.to_vec()).expect("Need valid UTF8");
-				Callback::get()(&origin, s.parse().expect("Expected an u32"));
+				if let Err(()) = Callback::get()(&origin, s.parse().expect("Expected an u32")) {
+					return Err(ProcessMessageError::Corrupt)
+				}
 			}
 			let mut m = MessagesProcessed::get();
 			m.push((message.to_vec(), origin));
@@ -198,7 +199,7 @@ impl ProcessMessage for RecordingMessageProcessor {
 }
 
 parameter_types! {
-	pub static Callback: Box<fn (&MessageOrigin, u32)> = Box::new(|_, _| {});
+	pub static Callback: Box<fn (&MessageOrigin, u32) -> Result<(), ()>> = Box::new(|_, _| { Ok(()) });
 	pub static IgnoreStackOvError: bool = false;
 }
 
@@ -253,7 +254,9 @@ impl ProcessMessage for CountingMessageProcessor {
 		if meter.try_consume(required).is_ok() {
 			if let Some(p) = message.strip_prefix(&b"callback="[..]) {
 				let s = String::from_utf8(p.to_vec()).expect("Need valid UTF8");
-				Callback::get()(&origin, s.parse().expect("Expected an u32"));
+				if let Err(()) = Callback::get()(&origin, s.parse().expect("Expected an u32")) {
+					return Err(ProcessMessageError::Corrupt)
+				}
 			}
 			NumMessagesProcessed::set(NumMessagesProcessed::get() + 1);
 			Ok(true)

substrate/frame/message-queue/src/tests.rs

@@ -1675,6 +1675,7 @@ fn regression_issue_2319() {
 	build_and_execute::<Test>(|| {
 		Callback::set(Box::new(|_, _| {
 			MessageQueue::enqueue_message(mock_helpers::msg("anothermessage"), There);
+			Ok(())
 		}));
 
 		use MessageOrigin::*;
@@ -1695,23 +1696,26 @@ fn regression_issue_2319() {
 #[test]
 fn recursive_enqueue_works() {
 	build_and_execute::<Test>(|| {
-		Callback::set(Box::new(|o, i| match i {
-			0 => {
-				MessageQueue::enqueue_message(msg(&format!("callback={}", 1)), *o);
-			},
-			1 => {
-				for _ in 0..100 {
-					MessageQueue::enqueue_message(msg(&format!("callback={}", 2)), *o);
-				}
-				for i in 0..100 {
-					MessageQueue::enqueue_message(msg(&format!("callback={}", 3)), i.into());
-				}
-			},
-			2 | 3 => {
-				MessageQueue::enqueue_message(msg(&format!("callback={}", 4)), *o);
-			},
-			4 => (),
-			_ => unreachable!(),
+		Callback::set(Box::new(|o, i| {
+			match i {
+				0 => {
+					MessageQueue::enqueue_message(msg(&format!("callback={}", 1)), *o);
+				},
+				1 => {
+					for _ in 0..100 {
+						MessageQueue::enqueue_message(msg(&format!("callback={}", 2)), *o);
+					}
+					for i in 0..100 {
+						MessageQueue::enqueue_message(msg(&format!("callback={}", 3)), i.into());
+					}
+				},
+				2 | 3 => {
+					MessageQueue::enqueue_message(msg(&format!("callback={}", 4)), *o);
+				},
+				4 => (),
+				_ => unreachable!(),
+			};
+			Ok(())
 		}));
 
 		MessageQueue::enqueue_message(msg("callback=0"), MessageOrigin::Here);
@@ -1735,6 +1739,7 @@ fn recursive_service_is_forbidden() {
 			// This call will fail since it is recursive. But it will not mess up the state.
 			assert_storage_noop!(MessageQueue::service_queues(10.into_weight()));
 			MessageQueue::enqueue_message(msg("m2"), There);
+			Ok(())
 		}));
 
 		for _ in 0..5 {
@@ -1778,6 +1783,7 @@ fn recursive_overweight_while_service_is_forbidden() {
 				),
 				ExecuteOverweightError::RecursiveDisallowed
 			);
+			Ok(())
 		}));
 
 		MessageQueue::enqueue_message(msg("weight=10"), There);
@@ -1800,6 +1806,7 @@ fn recursive_reap_page_is_forbidden() {
 		Callback::set(Box::new(|_, _| {
 			// This call will fail since it is recursive. But it will not mess up the state.
 			assert_noop!(MessageQueue::do_reap_page(&Here, 0), Error::<Test>::RecursiveDisallowed);
+			Ok(())
 		}));
 
 		// Create 10 pages more than the stale limit.
@@ -1976,83 +1983,29 @@ fn execute_overweight_keeps_stack_ov_message() {
 	});
 }
 
-/// Test that process_message is transactional
 #[test]
-fn test_process_message_transactional() {
-	use MessageOrigin::*;
+fn process_message_error_reverts_storage_changes() {
 	build_and_execute::<Test>(|| {
-		// We need to create a mocked message that first reports insufficient weight, and then
-		// `StackLimitReached`:
-		IgnoreStackOvError::set(true);
-		MessageQueue::enqueue_message(msg("stacklimitreached"), Here);
-		MessageQueue::service_queues(0.into_weight());
-
-		assert_last_event::<Test>(
-			Event::OverweightEnqueued {
-				id: blake2_256(b"stacklimitreached"),
-				origin: MessageOrigin::Here,
-				message_index: 0,
-				page_index: 0,
-			}
-			.into(),
-		);
-		// Does not count as 'processed':
-		assert!(MessagesProcessed::take().is_empty());
-		assert_pages(&[0]);
+		assert!(!sp_io::storage::exists(b"key"), "Key should not exist");
 
-		// create a storage
-		let vec_to_set = vec![1, 2, 3, 4, 5];
-		sp_io::storage::set(b"transactional_storage", &vec_to_set);
-
-		// Now let it return `StackLimitReached`. Note that this case would normally not happen,
-		// since we assume that the top-level execution is the one with the most remaining stack
-		// depth.
-		IgnoreStackOvError::set(false);
-		// Ensure that trying to execute the message does not change any state (besides events).
-		System::reset_events();
-		let storage_noop = StorageNoopGuard::new();
-		assert_eq!(
-			<MessageQueue as ServiceQueues>::execute_overweight(3.into_weight(), (Here, 0, 0)),
-			Err(ExecuteOverweightError::Other)
-		);
-		assert_last_event::<Test>(
-			Event::ProcessingFailed {
-				id: blake2_256(b"stacklimitreached").into(),
-				origin: MessageOrigin::Here,
-				error: ProcessMessageError::StackLimitReached,
-			}
-			.into(),
-		);
-		System::reset_events();
-		drop(storage_noop);
+		Callback::set(Box::new(|_, _| {
+			sp_io::storage::set(b"key", b"value");
+			Err(())
+		}));
 
-		// because the message was processed with an error, transactional_storage changes wasn't
-		// commited this means storage was rolled back
-		let stored_vec = sp_io::storage::get(b"transactional_storage").unwrap();
-		assert_eq!(stored_vec, vec![1, 2, 3, 4, 5]);
+		MessageQueue::enqueue_message(msg("callback=0"), MessageOrigin::Here);
+		MessageQueue::service_queues(10.into_weight());
 
-		// Now let's process it normally:
-		IgnoreStackOvError::set(true);
-		assert_eq!(
-			<MessageQueue as ServiceQueues>::execute_overweight(1.into_weight(), (Here, 0, 0))
-				.unwrap(),
-			1.into_weight()
-		);
+		assert!(!sp_io::storage::exists(b"key"), "Key should have been rolled back");
+	});
+}
 
-		// transactional storage changes, this means storage was committed
-		let stored_vec = sp_io::storage::get(b"transactional_storage").unwrap();
-		assert_eq!(stored_vec, vec![1, 2, 3]);
+#[test]
+fn process_message_ok_false_keeps_storage_changes() {
+	// FAIL-CI TODO
+}
 
-		assert_last_event::<Test>(
-			Event::Processed {
-				id: blake2_256(b"stacklimitreached").into(),
-				origin: MessageOrigin::Here,
-				weight_used: 1.into_weight(),
-				success: true,
-			}
-			.into(),
-		);
-		assert_pages(&[]);
-		System::reset_events();
-	});
+#[test]
+fn process_message_ok_true_keeps_storage_changes() {
+	// FAIL-CI TODO
 }