Venus: Increase unit test coverage to 80%

[dignifiedquire]

Description

We want to add tests to increase coverage to 80% and gain certainty over our code. Currently we are between 60-70%. Use your judgment regarding which tests to add.

Acceptance criteria

Please breakup PRs by package

Ordered in prioritiy:

• [ ] types
• [ ] core
• [ ] node
• [ ] abi
• [ ] commands
• [ ] mining
• [ ] wallet
• [ ] config
• [ ] repo

Risks + pitfalls

• See (inconclusive) discussion below on coverage vs. focus
• Avoid tests that lock in a specific implementation; test I/O instead (see contributing.md)

Where to begin

[victorb]

Focusing on code coverage metrics doesn't actually mean you're testing what should be tested. You could have one function and one test which tests that function, now you have 100% test coverage but if you happen to pass a string instead of int (just silly example), the function is broken.

Rather, try to focus testing the areas where testing is actually needed. You have a function that adds two numbers? Not really high-impact (nor effort) to test it. But for the functions that validates a transaction needs to have more than 100% of test coverage to be properly tested.

Might not be in my power to affect filecoin development / testing, but I've seen too many times a misdirected focus on code coverage metrics rather than covering high-impact logic.

[dignifiedquire]

Thanks @VictorBjelkholm, we have been thinking about testing for some time, and I agree with you that 100% coverage does not protect you from logic issues. If you want to see more details on discussions around testing take glimpse at #33.

This issue is here to make sure that the code we have is in good order and requiring a baseline of coverage is especially valuable to ensure that not only happy paths are tested from the get go.

[phritz]

I think coverage is a useful metric, but obviously not something to be obsessed about. I find both overall metrics and metrics for PRs to be useful signals.

You have a function that adds two numbers? Not really
high-impact (nor effort) to test it.

On the other hand if it potentially affects billions of dollars of value the 30 seconds it takes to write a test is probably worth the peace of mind. I've definitely seen functions "too simple to test" be the source of real bugs.

[laser]

If we are interested in increasing the certainty we have over our code through automated testing, I can think of a few different tools/approaches which might help us out:

1. Write table-driven or example tests in which we exercise a function or method with some inputs that we dream up. It seems to me to be the case that this the most commonly-encountered type of test in go-filecoin.
2. Write property-based tests in which we define random input-generating functions and use the output of those functions as inputs to our tests to exercise a function or method, verifying that some invariant holds. The test tool generates entropy, and entropy helps us exercise code paths that we didn't realize that we hadn't tested with a table-based test.
3. Use a mutation testing tool to find parts of our implementation which are not exercised sufficiently by tests. The mechanism is simple: The tool changes your software, runs your tests, and verifies that the change results in a test failure. We could use a mutation testing tool in combination with property-based and table-based/example tests.

Questions for the group:

1. Are we open to experimenting with a property-based testing package, e.g. testing/quick?
2. Are we open to experimenting with a mutation testing tool, e.g. go-mutesting?

Caveats:

1. I have not used Golang's testing/quick package, but I have used a similar property-based testing library for Haskell, QuickCheck.
2. I have not used Golang's zimmski/go-mutesting package, but I have used a similar mutation testing tool for Java, PIT.

[dignifiedquire]

I think both property based tests and mutation tests are great approaches to ensure quality. They also should make it a lot easier to add meaningful tests that can survive refactors that don't change the api surface.

To move forward with this issue I would suggest to use table-drive tests in combination with exploring testing/quick.

Doing mutation testing is great, but I would punt setting that up to a later point when the code is a bit more stable and we are adding the next layer of security.

[phritz]

We should make sure our code coverage excludes test helpers, which are generally (but not always) untested. They generally go in testing.go files. Edit: this is done

[phritz]

Probably useful to see where our coverage has dropped, for example here:

https://codecov.io/gh/filecoin-project/go-filecoin/branch/master/commits?from=2018-03-14%2023%3A00%3A00&to=2018-03-15%2016%3A00%3A00

we can see this change seems to have dropped it significantly: https://codecov.io/gh/filecoin-project/go-filecoin/commit/034c8b5e09b880905a59d2f65bf826da49488dcc

[mishmosh]

Turn this into an epic, with 1 story per package.

[phritz]

• timebox to 2-3 points
• prolly throw off another issue for the 2-3 point effort
• consider starting in commands where most of the functionality is only command tested and not unit tested

[phritz]

Closing this out, no longer a goal.