Improve testing in Q4

[jakmeier]

Description

In the Q3 retro, it has come up that the team isn't particularly happy with what gets caught in CI vs what issues we find in dev net.

In a focus meeting last week, we then narrowed it down further. Test coverage of the happy-path for most features is not the problem. These are well covered by integration tests. The problems are mostly around:

• We lack testing cases involving non-ideal behavior. Examples are long delays, offline nodes, indexers behaving badly, or generators hitting limits of concurrent generations.
• When identifying a bug, it often is a lot of work to create a test case for it.

Both of these are related. Integration tests aren't well-suited to force a specific misbehavior somewhere in an inner system component.

One goal this quarter is to improve our ability to test such corner cases. Based on the last team discussion, the primary focus should be on code where we are also adding new features. In particular, never-failing MPC request processing. Probably also anything to do with bidirectional messaging.

Possible Improvements

Here are a few concrete items that we can work on to achieve our goal:

• Better test fixture network manipulation: Component testing already allows to control the network behavior in a basic way, such as dropping certain messages. We should use this to add more test cases and simultaneously build out the testing framework to make it easier to control cross-node messaging.
• Better test result checks: Checking specific outcome in MPC component testing is currently still annoying and relies on list of produced actions typed as String. We should make it easier to check what exactly the system did at the end of a test.
• Component testing for indexers: We want a way to test indexer logic in corner cases, too. Adding a kind of component testing for indexers specifically could be the way forward here.
• Bidirectional corner case testing: Right now, we only have component testing for the core MPC functionality. Bidirectional messaging adds more logic that's between simple indexing of chains and MPC signing. Let's add test cases for all known corner cases, in what ever framework makes the most sense.

[jakmeier]

Better test result checks: Checking specific outcome in MPC component testing is currently still annoying and relies on list of produced actions typed as String. We should make it easier to check what exactly the system did at the end of a test.

With #584, I have a first proposed step forward for this.

Building on top of the added trait CollectMessages, I have an idea how we could implement snapshot testing.

• An implementation of trait CollectMessages can collect all messages in one big multi-line string. Messages need to be "redacted" in a way that non-deterministic numbers are not included. This string is the snapshot.
• At the end of a test, we access this snapshot string and ensure it doesn't change across test runs. This is trivially done with snapshot testing frameworks, personally I would use expect_test with expect_file!.
• Now we can add tests by simply defining a bunch of weird scenarios and only need a one-liner like expect_file!('malicious_case_1.json').assert_eq(&snapshot) to do the checks in the end. Run it once, look at the auto-generated snapshot file for a sanity check, then rerun and to see if the output is deterministic. Done.

I think this would work great to add test cases for timeout into retries under various conditions. Coming up with manual checks that the retry happened as expected can be nasty. This approach would make it as easy as looking at the snapshot once to confirm it did the retry.

@ChaoticTempest WDYT?

[ChaoticTempest]

so with CollectMessages, we'd end up just testing against a log file right and just expect a set of strings to appear?

And then expect! just updates at runtime (very magical), so we don't end up caring if the logs are changed by much? I think if it works, then would be pretty useful and make some cases easier to test.

[jakmeier]

So, trait CollectMessages can collect structured data, not just a logfile. But for expect!, we would reduce that data to a string / logfile.

expect! just updates at runtime (very magical)

So the expect! macro takes a fixed string as input. No runtime magic happens in a usual test run. But instead of manually updating the string, you can use UPDATE_EXPECT=1 which enables the magic.

The magic is that the macro is aware of where in the code the macro was called, which allows updating the string in that exact location automatically. It uses the same kind of macro tooling as when you see the source code line number in a panic /error log message.

so we don't end up caring if the logs are changed by much

That logic we need to add manually in how we produce the string for comparison. expect! only does strict comparisons.

[volovyks]

This idea can be useful, what scenarios do you have in mind @jakmeier?
I do not think we should use it heavily, since we are not ensuring that "everything is correct", we only check that "everything is the same as it was before".

[jakmeier]

I think it would be useful, for example, any time we find a new scenario of message interactions that we didn't handle properly. This approach should make it easy to check each node does what it is expected to do.

we are not ensuring that "everything is correct", we only check that "everything is the same as it was before"

That's not entirely true. If the output string is generated well, it contains all information necessary (usually even more than necessary) to confirm everything a list of assert! would test anyway.

[volovyks]

What I'm saying, is that we are not sure that there is no mistakes in the original snapshot.

[jakmeier]

Ah yes. We still have to manually verify the snapshot is correct when adding it initially.

Hopefully, verifying a snapshot is easier than writing assertions manually.

[jakmeier]

Q4 is over. Here is a quick reflection.

On the positive, we have added more tests now that also check specific corner cases.
Component testing worked for new features we added, as well as for reproducing difficult bugs. So I see this as a win.

On the negative, we haven't extended component testing to indexer logic, which largely remains untested outside of the "old" integration tests that really only test the happy path.

Similarly, testing for bidirectional signing has not been included in component testing (, yet).