stake: Minor improvements in IsTreasuryX tests.

[jholdstock]

I was late to the party reviewing these recent changes but I've taken a look anyway and here are a few minor things I noticed could be improved.

Using t.Run takes advantage of go tooling to prefix test error messages with the test name, rather than having to manually constructing the strings.

[davecgh]

I really don't like t.Run for these cases at all. It makes the test names horrible in the output and makes the VSCode testing output messy with a ton of extra junk in the treeview. I probably should start calling them desc instead of name to avoid the temptation, because they're really descriptions (per the comment), not names.

It also pushes the test code that actually matters most further to the right.

As a case in point:

=== RUN   TestTreasuryIsFunctions
=== RUN   TestTreasuryIsFunctions/treasury_add_from_user_with_change
=== RUN   TestTreasuryIsFunctions/treasury_add_from_user_with_no_change
=== RUN   TestTreasuryIsFunctions/treasury_add_from_user_with_OP_RETURN
=== RUN   TestTreasuryIsFunctions/treasury_add_from_treasurybase
=== RUN   TestTreasuryIsFunctions/treasury_spend_p2sh
=== RUN   TestTreasuryIsFunctions/treasury_spend_p2pkh
=== RUN   TestTreasuryIsFunctions/treasury_spend_invalid_output_1_p2pk_(not_p2sh/p2pkh)
--- PASS: TestTreasuryIsFunctions (0.00s)
    --- PASS: TestTreasuryIsFunctions/treasury_add_from_user_with_change (0.00s)
    --- PASS: TestTreasuryIsFunctions/treasury_add_from_user_with_no_change (0.00s)
    --- PASS: TestTreasuryIsFunctions/treasury_add_from_user_with_OP_RETURN (0.00s)
    --- PASS: TestTreasuryIsFunctions/treasury_add_from_treasurybase (0.00s)
    --- PASS: TestTreasuryIsFunctions/treasury_spend_p2sh (0.00s)
    --- PASS: TestTreasuryIsFunctions/treasury_spend_p2pkh (0.00s)
    --- PASS: TestTreasuryIsFunctions/treasury_spend_invalid_output_1_p2pk_(not_p2sh/p2pkh) (0.00s)

Compared to the current:

=== RUN   TestTreasuryIsFunctions
--- PASS: TestTreasuryIsFunctions (0.01s)
PASS

Now, multiply that by 1000s of tests with 10+ cases throughout the code base and it quickly gets unwieldy.

The way it is now, you only see the case name/description when something goes wrong, which is the only time it actually matters.

[jholdstock]

Fair point, I'll undo that. I think my opinion differs here because I am never interested in the output of a successful test run. I only pay attention when tests fail, and in those cases I believe the output of using t.Run is more helpful than not using it.

I don't think I've even seen that tree view before. I'm going to dig into that because it looks like it has some good features (sorting by duration could be very nice for optimizing things).

[davecgh]

Interesting. I'm curious to hear your perspective on how the output of using t.Run is more helpful? I haven't used it all that much due to the aforementioned reasons, so it's likely there is some aspect(s) I'm not aware of.

If we purposely break the code to see failures:

With the current code:

--- FAIL: TestTreasuryIsFunctions (0.00s)
    treasury_test.go:279: treasury add from user with change: unexpected treasury add result - got false, want true   
    treasury_test.go:279: treasury add from user with no change: unexpected treasury add result - got false, want true
FAIL
exit status 1
FAIL    github.com/decred/dcrd/blockchain/stake/v5      0.434s

With t.Run:

$ go test -run TestTreasuryIsFunctions
--- FAIL: TestTreasuryIsFunctions (0.00s)
    --- FAIL: TestTreasuryIsFunctions/treasury_add_from_user_with_change (0.00s)   
        treasury_test.go:280: unexpected treasury add result - got false, want true
    --- FAIL: TestTreasuryIsFunctions/treasury_add_from_user_with_no_change (0.00s)
        treasury_test.go:280: unexpected treasury add result - got false, want true
FAIL
exit status 1
FAIL    github.com/decred/dcrd/blockchain/stake/v5      0.441s

I personally even prefer the failure output of the current code in that case too because it's less noise and I can copy the exact test name/desc string and search for it instead of having to undo all of the underscore decorations and manually figure out which one it is.