New collections will fail if context left as `is_view` at end of unit test

[austinabell]

So I've noticed a bit of a footgun with our current test patterns relating to unit tests.

Previously collection types would not write themselves back to state as this had to be handled manually, so the unit test:

 testing_env!(VMContextBuilder::new().is_view(true).build());
 let contract = MyContract::new(); 
 assert_eq!(None, contract.some_method(...));

Would work totally fine because even though view context is true, the initialization never actually wrote anything to state (even though it would when sending a tx in wasm).

The issue is now we have collections that will write their values to store on drop, so if MyContract has one of these:

#[near_bindgen]
#[derive(BorshDeserialize, BorshSerialize, Default)]
pub struct StatusMessage 

{ some_value: Lazy<u8>, } 

...
    #[init]
    pub fn new() -> Self {
        Self 

         { unique_values: Lazy::new(b"v", 8), } 

     }

then the initialized value with MyContract::new will make the state modification but since the contract is not dropped (like it would be in a wasm env) then the value will actually be written at the end of the test scope. This is an issue if a developer left the mocked env to read-only like in the case above.

The issue is that the initialization function doesn't write to storage when it should and that writes will all happen at the end of the function, which doesn't match the flow of the actual contract execution. This is the same also for &mut functions, since with these collections now the values won't be written to state immediately.

A possible solution to this is to add some codegen for init and mutable functions, only for non-wasm32 environments, that will serialize this state, write it to state with env::state_write in the case of init functions, then deserialize it into the type that is returned or replace the &mut self. This is a bit of a janky pattern but it will change the code to be a closer reflection of what actually happens in wasm/on-chain.

Very much open to alternatives though.

[austinabell]

Also noticed there is a bit of a consideration with this same pattern happening with using these new collections as statics. With something like thread_local!, the logic is run on every method call to initialize the static, but the value is never dropped before the runtime exits. Experimented with this a bit last night https://github.com/austinabell/static_lookup_map/blob/main/src/lib.rs and just switched to using the old LookupMap, but if switched to new, there is a requirement to manually flush the collections after each operation.

This is also the same type of foot gun that would be very hard to debug if you don't understand what is happening internally with these. I'm not quite sure if there is a better alternative if someone wants to do this pattern, but if there is not, we should at least document this interaction somewhere.

[frol]

Here is a full reproducible example:

use near_sdk::{log, near};

#[near(contract_state)]
pub struct Contract {
    greeting: near_sdk::store::LookupMap<String, String>,
}

impl Default for Contract {
    fn default() -> Self {
        Self {
            greeting: near_sdk::store::LookupMap::new(b"g"),
        }
    }
}

#[near]
impl Contract {
    pub fn get_greeting(&self) -> String {
        self.greeting.get("greeting").unwrap_or(&"NOT SET YET".to_string()).clone()
    }

    pub fn set_greeting(&mut self, greeting: String) {
        log!("Saving greeting: {greeting}");
        self.greeting.insert("greeting".to_string(), greeting);
    }
}

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn set_then_get_greeting() {
        let mut contract = Contract::default();
        contract.set_greeting("howdy".to_string());
        // Commenting out this line "helps", but it does not enforce the View context limitations to the `get_greeting` function
        near_sdk::testing_env!(near_sdk::test_utils::VMContextBuilder::new().is_view(true).build());
        // If you add `contract.greeting.flush()` before the `testing_env!` line, the test will also run fine.
        // `drop(contract)` is another way to trigger all the `.flush()` methods.
        assert_eq!(contract.get_greeting(), "howdy");
    }
}

Run:

cargo test

[renzobanegass]

Isn't using a manual flush a valid approach? We could document the need to flush state manually in tests. I think this approach is simple and keeps the code reflective of the actual contract behavior.

Just a proposal, maybe a more complex approach is the way to go.

[frol]

That will require flushing all the individual collections (e.g. see this contract, I don't want to flush 10+ fields manually).

We need to trigger Drop behavior on the Contract somehow to call Drop on all the underlying collections to flush them all. Maybe we need to change the structure of unit-tests or testing_env! logic.

[renzobanegass]

I get what you are saying. What about using scoped blocks, when getting out of scope the contract triggers the Drop behavior. Then you can switch to view context and reload the contract to make the assertions, it would look something like this on the example you wrote:

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn set_then_get_greeting() {
        {
            let mut contract = Contract::default();
            contract.set_greeting("howdy".to_string());
        }        

         near_sdk::testing_env!(near_sdk::test_utils::VMContextBuilder::new().is_view(true).build());

        let contract = Contract::default();
        assert_eq!(contract.get_greeting(), "howdy");
    }
}

This passes the test:

[frol]

@renzobanegass Scoped blocks are nice, but users might get confused about the error they would be getting since it looks like get_greeting is broken and tries to write something to the storage, which is not the case.

Any ideas on how to make this ergonomic and clear? We may think in several directions: improving the error message, make the intuitive code work as expected, make the intuitive code fail with a clear message on how to properly structure the tests.

[renzobanegass]

I'm not sure about modifying testing_env as it has other use cases. What about creating a new macro for the tests that wraps testing_env and drops the contract before setting the environment? I can't come up with a way to implement it in an intuitive way though.