Fix runtime tests broken by fees. Fixes #1185

[MaksymZavershynskyi]

Fixes #1185 .
Runtime tests are too wide and assert irrelevant numbers, like they assert that hash of the account did not change and the exact balance of the account (which is problematic now, since we apply fees). These unit tests are actually integration tests and we should have very few integration tests because they have maintenance cost. Instead unit tests should be verifying one or few specific numbers. Also our unit tests are too large, see the following blog posts for good practices:
https://testing.googleblog.com/2018/06/testing-on-toilet-keep-tests-focused.html
https://testing.googleblog.com/2018/06/testing-on-toilet-only-verify-relevant.html

[MaksymZavershynskyi]

CC @bowenwang1996 regarding the links above.

[bowenwang1996]

why?

[MaksymZavershynskyi]

It explains why.

[bowenwang1996]

I don't understand why it cannot account for all fees.

[MaksymZavershynskyi]

These tests have problems in general, please read the attached blog posts:

• It is not clear what the unit test is actually testing. It checks for all kinds of values, including nonces, hashes, balances, etc;
• We should have few integration tests and even fewer end-to-end tests, because they require a lot of maintenance -- a lot of changes to the domain logic require modifications to integration tests and end-to-end tests. These unit tests here are actually integration tests in disguise;
• These unit tests are too large and check everything, see: https://testing.googleblog.com/2018/06/testing-on-toilet-keep-tests-focused.html
• If we want to test the side effect of staking on the balance we should be checking it in the module responsible for staking, e.g. ValidatorManager . The side effect of rent should be checked in module that applies rent. The fees should be checked in near-vm-runtime.
• Some fees (like storage rent) have non-deterministic nature. We cannot assert a specific balance, because it depends on a multitude of external factors, like how many blocks have passed;
• Threading such information will violate encapsulation. near/runtime.rs does not have an access to the economics config at the moment, because it is encapsulated in the runtime crates. Exposing such config would break encapsulation.

[bowenwang1996]

These tests are supposed to check that the balances check out exactly. If we don't check it here, where do we put the tests that make sure all the balances are correctly calculated?

[MaksymZavershynskyi]

These tests are supposed to check that the balances check out exactly. If we don't check it here, where do we put the tests that make sure all the balances are correctly calculated?

In the code that changes the balances. Specifically, the runtime near-vm-logic should be checking that amounts related to fees are correctly processed. And the code related to staking, e.g. ValidatorManager should be checking that staking is correctly processed. We cannot have one catch-all test (which is currently also disguised as a unit test) that asserts every single side effect on the balance of an account.

[bowenwang1996]

Okay. Do we assume everything would work together once each part is tested or is there some other plan to test that all the accountings work together properly?

[MaksymZavershynskyi]

Okay. Do we assume everything would work together once each part is tested or is there some other plan to test that all the accountings work together properly?

There is a pyramid of testing: num unit tests >> num integration tests >> num end to end tests.

Integration tests cannot be checking for all specific behavior of the system, instead they should focus on generic behavior, e.g. the validator was not kicked out. We can have very few integration tests that check specific numbers, like nonces, specific balances, etc, because they are hard to maintain, though easy to write.