[Tooling] Flaky tests - Issue #192

[deblasis]

Description

This PR addresses test flakiness due to:

• race conditions
• use of timers that causes non-determinism

Tests are still flaky

TestHotstuff4Nodes1BlockHappyPath

for i in {1..10}; do go test -timeout 30s -count=1 -run ^TestHotstuff4Nodes1BlockHappyPath$ github.com/pokt-network/pocket/consensus/consensus_tests; done | egrep  '^\--- FAIL:|ok '

TestTinyPacemakerTimeouts

for i in {1..10}; do go test -timeout 30s -count=1 -run ^TestTinyPacemakerTimeouts$ github.com/pokt-network/pocket/consensus/consensus_tests; done | egrep  '^\--- FAIL:|ok '

Time mocking

Following the input in this comment I proceeded implementing time mocking.

The idea is that the nodes will have a reference that can proxy calls to either the real time package or a mocked version.
I am using github.com/benbjohnson/clock and I am starting to build utility functions around it that could become a wrapper at some point but it's early to say.

Something like this:

I believe this functionality is going to be quite useful in general especially since I believe we'll have to deal with timeouts and time-bound stuff quite a bit.

Race conditions

Mostly due to the fact that structs are accessed by multiple goroutines at the same time.

Addressed using mutexes and wrapping the getters/setters into methods that also handle lock/unlock.

Issue

Fixes issue/192

Type of change

Please mark the options that are relevant.

• New feature, functionality or library
• Bug fix (non-breaking change which fixes an issue)
• Code health or cleanup
• Other: Investigation

List of changes

• Introduced time mocking
• Fixed race conditions in Consensus module

Testing

• make test_all
• LocalNet w/ all of the steps outlined in the README

Checklist

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have tested my changes using the available tooling
• If applicable, I have made corresponding changes to related local or global README
• If applicable, I have added new diagrams using mermaid.js
• If applicable, I have added tests that prove my fix is effective or that my feature works

[deblasis]

getting somewhere

[deblasis]

10 runs with and without the -race flag and no flakiness detected 🙌

[deblasis]

Hey @okdas! Can you please review this specific commit?

6e0b00f

I made sure we skip the dockerhub steps that currently are simply failing because not fully setup.

This way we don't get false negatives in CI runs.

[Olshansk]

General: I did a first pass at the review and think this is super interesting. It has the foundation for important infrastructure we'll really need in the future when we do more extensive testing. 🧑‍🔬 🤔 🧪

Will do a second pass after we tend to the first set of comments.

Random Ideas: Thoughts on creating PocketClock module. Not creating an issue for it (yet) since it's just an idea, but wanted to see if you think it'd be overkill.

Primary concern: I did some googling and it seems that Setters and Getters in go are idiomatic, but I'm concerned that:

1. We need a way to enforce/encourage this because of the mutex requirements (you can imagine the issues as we have more contributors)
2. Grep & update all existing sets/gets

[okdas]

I made sure we skip the dockerhub steps that currently are simply failing because not fully setup.

This should really not happen because this part is set up for sure and usually works, but then sometimes docker hub does not authenticate and throws an error. I'll check why that might happen. Thank you for calling this out.

[Olshansk]

I think we're almost on the home stretch!!

Also, for future reference, you should be able to create branches directly from the main repo; deblasis:issue/192-tooling-flaky-tests.