State Not Reverted in Unit Tests when Error is Returned

[peterwht]

Describe the bug
When a message / function returns a Result::Err any modified state up to that point should be reverted. When running on a live-chain this DOES happen. However, when this occurs while running unit tests (cargo test) the state is NOT reverted.

To clarify:

• off-chain unit testing does NOT revert the state <- this is the problem
• Testing on on-chain DOES revert the state
• e2e testing (on-chain) DOES revert the state

Please see the following repo to reproduce the issue: https://github.com/peterwht/ink-unit-test-bug/tree/main

The following code snippet showcases the issue

        #[ink(message)]
        pub fn flip_with_error(&mut self) -> Result<(), FlipError>{
            self.value = !self.value;
            // Revert should occur and self.value should remain unchanged
            Err(FlipError::FlipError)
        }

// ...

        #[ink::test]
        fn it_works() {
            let mut unit_test_bug = UnitTestBug::new(false);
            assert_eq!(unit_test_bug.get(), false);
            // Error is returned, revert should occur, and value should remain as false
            assert_eq!(unit_test_bug.flip_with_error(), Err(FlipError::FlipError));
            // This test is going to FAIL because the revert did not occur. It should pass
            assert_eq!(unit_test_bug.get(), false);
        }

Expected behavior
When running unit tests, all state should be reverted when an error is returned from the called function.

Additional context
ink! v4.2.0

[goastler]

I've hit this bug too in my unit tests, can replicate.

[ascjones]

I'm not too familiar with the unit testing code, but this may not be super simple to fix. It looks like the method calls are just using the state of the struct as is and not emulating reading and writing from storage as a transaction. It may involve changing the codegen to generate message calls which emulate the transactional nature of pallet-contracts, or we could switch to using the call builder to build messages and then dispatch them into the offchain engine. Then it begins to raise the question of whether it is worth maintaining such an emulator, or if we should focus on improving the E2E experience or perhaps running an embedded pallet-contracts somehow. @cmichi

[SkymanOne]

Can confirm the issue

[SkymanOne]

Looking at the off-chain testing engine, any state mutations occur on the Rust struct directly, as pointed out by Andrew. I don't see much motivation adding overhead complexity by trying to encapsulate the struct into some storage struct. This will essentially break the "lightness" and "transparentness" of the unit testing.

Unit tests are not intended to fully simulate the off-chain execution but rather test the functional correctness of the contract's logic.
If you want to simulate the on-chain execution without the need to run the node, this has been achieved by quasi-testing with off-chain E2E testing framework, drink!, which is shipping as part of ink! 5.0.0 release

[SkymanOne]

Closing due to inactivity