test: cli: chain category unit tests #8048

TheDivic · 2022-02-08T16:37:37Z

Proposed Changes

CLI actions lack unit tests. I decided to use the approach similar to what I found in send_test.go using gomock, but I don't rely on custom "service" implementations but mock the whole FullNode API.

What's in this PR:

Unit tests for most of the CLI methods in the chain category.
The GetFullNodeAPI constructor function checks if there's an injected mock API in the app Metadata and uses that in unit tests.
Actions don't use raw fmt.* but instead write to the app.Writer so the CLI output is testable
I replaced usages of os.File with a io.WriterCloser and implemented a construction function that check app metadata for an instance of WriterCloser before calling os.Create to prevent side-effects in unit tests.
Added some useful mock helper functions that can be used in other tests

Additional Info

These unit tests heavily rely on mocking and there’s some funky string matching but I think they are valuable because:

It will bring some significant increases to the overall test coverage which seems low (even though most of the important things are tested)
It will make sure that the output of the CLI commands stays the same so we don’t break the user space
If the underlying API changes the CLI tests will break, so we know we have to fix them.

Checklist

Before you mark the PR ready for review, please make sure that:

All commits have a clear commit message.
The PR title is in the form of of <PR type>: <area>: <change being made>
- example: fix: mempool: Introduce a cache for valid signatures
- PR type: fix, feat, INTERFACE BREAKING CHANGE, CONSENSUS BREAKING, build, chore, ci, docs,perf, refactor, revert, style, test
- area: api, chain, state, vm, data transfer, market, mempool, message, block production, multisig, networking, paychan, proving, sealing, wallet, deps
This PR has tests for new functionality or change in behaviour
If new user-facing features are introduced, clear usage guidelines and / or documentation updates should be included in https://lotus.filecoin.io or Discussion Tutorials.
CI is green

CLI actions lack unit tests. I decided to use the approach similar to what I found in `send_test.go` using gomock, but I don't rely on custom "service" implementations but mock the whole FullNode API. This first commit validates the test setup by testing the simplest method of the chain category, e.g. `chain head`. This requires a minor refactor of the CLI action code: - The constructor (`GetFullNodeAPI`) checks if there's an injected mock API in the app Metadata and uses that in unit tests. - Actions shouldn't use raw `fmt.*` but instead write to the `app.Writer` so the CLI output is testable

…to cli-chain-tests

codecov · 2022-02-08T16:41:06Z

Codecov Report

Merging #8048 (580fa86) into master (b27196b) will increase coverage by 0.33%.
The diff coverage is 67.77%.

@@            Coverage Diff             @@
##           master    #8048      +/-   ##
==========================================
+ Coverage   39.21%   39.54%   +0.33%     
==========================================
  Files         660      660              
  Lines       71436    71474      +38     
==========================================
+ Hits        28014    28266     +252     
+ Misses      38602    38294     -308     
- Partials     4820     4914      +94

Impacted Files	Coverage Δ
cli/chain.go	`39.94% <61.53%> (+39.94%)`	⬆️
chain/types/mock/chain.go	`84.00% <82.60%> (-2.67%)`	⬇️
cli/util/api.go	`24.27% <100.00%> (+0.74%)`	⬆️
chain/stmgr/call.go	`67.87% <0.00%> (-7.28%)`	⬇️
extern/sector-storage/worker_tracked.go	`79.09% <0.00%> (-7.28%)`	⬇️
chain/events/observer.go	`71.64% <0.00%> (-6.72%)`	⬇️
markets/retrievaladapter/client_blockstore.go	`62.50% <0.00%> (-6.25%)`	⬇️
storage/wdpost_sched.go	`75.49% <0.00%> (-5.89%)`	⬇️
extern/sector-storage/sched.go	`83.12% <0.00%> (-3.30%)`	⬇️
miner/miner.go	`55.40% <0.00%> (-2.96%)`	⬇️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b27196b...580fa86. Read the comment docs.

Unit test for the cli `chain getblock` command. Tests if output is JSON in the expected format.

magik6k · 2022-02-08T19:10:27Z

(converted to draft as the description notes that it's still WIP)

Simple test that checks if this CLI method prints the IPLD node referenced by the given CID encoded in hexadecimal.

Contains two subtests, that check if the --really-do-it flag (force) is respected, since removing wrong objects may lead to sync issues.

Test expected output with respect to the --base flag

I also added some helper functions for mocking in the types/mock pkg

Also moved the mock definition to a separate file (mocks_test.go) because it's gonna be used in other test files, and it didn't make sense for it to stay inside chain_test.go.

Some "funky" string matching in this one, but I think that's ok. Chain is love. ❤️

Cover the essential function execution paths, no time for every -as-type combination.

Modified ChainExportCmd to use io.WriterCloser instead of os.File so it the file can be mocked in unit tests, without side effects to the FS.

Contains some funny mocking logic, because estimate gas price is called multiple times (for various nblocks) and I wanted to make it as flexible as possible.

TheDivic · 2022-02-10T23:33:16Z

@magik6k I changed the status back to "Ready to Review" because it's as ready as it's ever gonna be 🙂

ZenGround0 · 2022-02-15T15:22:57Z

chain/types/mock/chain.go

@@ -96,3 +89,30 @@ func TipSet(blks ...*types.BlockHeader) *types.TipSet {
 	}
 	return ts
 }
+
+func RandomActorAddress() (*address.Address, error) {


In general I'm opposed to random artifacts in tests as they make reproducibility hard. What happens if we instead take a seed and derive something randomly off of that? Then if we want a lot of coverage of different values we can iterate.

If we want coverage over lots of addresses do the RandomActorAddress(seed) approach with deterministic seed and add loops over seed value into tests calling RandomActorAddress.

If we just want an arbitrary address make a simple default choice and stick with it. I think this is probably what we want.

Agreed. I've refactored the random generator to use a provided seed to generate the desired number of addresses.
We could make a default arbitrary address, but I think having a random address gen function can come in handy.

ZenGround0 · 2022-02-15T15:24:17Z

chain/types/mock/chain.go

+
+func RandomActorAddress() (*address.Address, error) {
+	bytes := make([]byte, 32)
+	_, err := rand.Read(bytes)


nit: If we do end up going with random artifacts we should probably use math since there is no need for crypto guarantees in tests. The interface doesn't even change so it's real easy to switch.

Switched to math.rand for the generator

ZenGround0 · 2022-02-15T15:35:58Z

cli/chain.go

@@ -199,7 +205,7 @@ var ChainReadObjCmd = &cli.Command{
 			return err
 		}

-		fmt.Printf("%x\n", obj)
+		afmt.Printf("%x\n", obj)


@magik6k any reason to not use the app writer for chain CLI?

ZenGround0 · 2022-02-15T15:53:16Z

cli/chain_test.go

+		ParentMessages []cid.Cid
+	}{}
+
+	err = json.Unmarshal(buf.Bytes(), &out)


Not sure if it's worth it but there's maybe some value in passing along specific values from the mocks rather than empty values and then checking that cli output data matches. This could catch a bug that makes a command only return empty valued data.

I like this idea. Checking for command outputs is a good enough solution, though it might miss bugs similar to the ones you have mentioned.
However, for this PR, I would leave the current solution as it stands, and we can refactor it in the future.

ZenGround0 · 2022-02-15T16:14:10Z

cli/chain_test.go

+		// output is plaintext, had to do string matching
+		assert.Contains(t, out, from.String())
+		assert.Contains(t, out, to.String())
+		assert.Contains(t, out, "Send")


This is going to match the line By Sender every time whereas I think you want to match the method type. "Send " (with a space) is an ok workaround. "By Method: \nSend" might be a bit better but you'll need to get the exact formatting down.

Maybe another regex matching Send with no [er] after

I think match both By Sender and Send is the best way to go in this test, so I added an extra regex match.

I don't think this works. "By Sender" will cause both asserts to pass, right?

You're right.
I changed the assertion check to verify the method separately, now.

ZenGround0 · 2022-02-15T17:25:53Z

cli/chain_test.go

+	blk.Height = 1
+	head := mock.TipSet(blk)
+
+	head.Height()


nit: unused?

Nice catch! Removed.

Refactor random addr generation to use a rand seed. Remove unused lines in tests.

brdji · 2022-02-17T13:43:28Z

Since @TheDivic is on vacation at the moment, I have taken the liberty of reviewing and fixing issues in the PR.

TheDivic added 2 commits February 8, 2022 17:15

Merge branch 'master' of https://github.com/filecoin-project/lotus in…

35918cb

…to cli-chain-tests

TheDivic requested review from TheMenko and brdji February 8, 2022 16:37

TheDivic requested a review from a team as a code owner February 8, 2022 16:37

test: cli chain getblock command

78649d4

Unit test for the cli `chain getblock` command. Tests if output is JSON in the expected format.

TheMenko approved these changes Feb 8, 2022

View reviewed changes

magik6k marked this pull request as draft February 8, 2022 19:09

TheDivic added 12 commits February 9, 2022 15:29

test: chain read-obj

1cd590a

Simple test that checks if this CLI method prints the IPLD node referenced by the given CID encoded in hexadecimal.

test: chain delete-obj cli command

c0f47e5

Contains two subtests, that check if the --really-do-it flag (force) is respected, since removing wrong objects may lead to sync issues.

test: chain stat-obj cli command

a923d7c

Test expected output with respect to the --base flag

test: chain getmessage cli command

e797ec1

I also added some helper functions for mocking in the types/mock pkg

test: chain sethead cli command

ae49729

Also moved the mock definition to a separate file (mocks_test.go) because it's gonna be used in other test files, and it didn't make sense for it to stay inside chain_test.go.

test: chain inspect-usage cli command

4e37131

test: chain list (love) cli command

b3f7db7

Some "funky" string matching in this one, but I think that's ok. Chain is love. ❤️

test: chain get cli command

6bc2ee2

Cover the essential function execution paths, no time for every -as-type combination.

test: chain bisect cli command

b536dfa

test: chain export cli command

c0f89e5

Modified ChainExportCmd to use io.WriterCloser instead of os.File so it the file can be mocked in unit tests, without side effects to the FS.

test: chain gas-price cli command

e5ac866

Contains some funny mocking logic, because estimate gas price is called multiple times (for various nblocks) and I wanted to make it as flexible as possible.

cleanup: small cleanup before final push

107eb76

TheDivic changed the title ~~[WIP] test: cli: chain category unit tests~~ test: cli: chain category unit tests Feb 10, 2022

TheDivic requested a review from magik6k February 10, 2022 23:18

TheDivic marked this pull request as ready for review February 10, 2022 23:32

TheMenko mentioned this pull request Feb 11, 2022

test: cli: adding wallet tests #8079

Merged

5 tasks

TheDivic mentioned this pull request Feb 11, 2022

test: cli: unit tests for sync related commands #8080

Merged

5 tasks

doc: add stm annotation to cli chain tests

e6c8c9a

ZenGround0 approved these changes Feb 15, 2022

View reviewed changes

Fix PR comments.

6feae19

Refactor random addr generation to use a rand seed. Remove unused lines in tests.

Darko Brdareski added 2 commits February 21, 2022 12:33

Add stm file annotation

87c63e2

Fix matching for TestInspectUsage

675012f

TheMenko mentioned this pull request Feb 21, 2022

test: cli: adding network cli tests #8161

Open

5 tasks

brdji mentioned this pull request Feb 21, 2022

test: Cli: add mempool tests #8162

Merged

5 tasks

Change annotation to #cli

580fa86

ZenGround0 merged commit 6123aa2 into master Feb 22, 2022

ZenGround0 deleted the cli-chain-tests branch February 22, 2022 02:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: cli: chain category unit tests #8048

test: cli: chain category unit tests #8048

TheDivic commented Feb 8, 2022 •

edited

codecov bot commented Feb 8, 2022 •

edited

magik6k commented Feb 8, 2022

TheDivic commented Feb 10, 2022

ZenGround0 Feb 15, 2022

ZenGround0 Feb 15, 2022

brdji Feb 17, 2022

ZenGround0 Feb 15, 2022

brdji Feb 17, 2022

ZenGround0 Feb 15, 2022

ZenGround0 Feb 15, 2022

brdji Feb 17, 2022

ZenGround0 Feb 15, 2022

ZenGround0 Feb 15, 2022

brdji Feb 17, 2022

ZenGround0 Feb 21, 2022

brdji Feb 21, 2022

ZenGround0 Feb 15, 2022

brdji Feb 17, 2022

brdji commented Feb 17, 2022

test: cli: chain category unit tests #8048

test: cli: chain category unit tests #8048

Conversation

TheDivic commented Feb 8, 2022 • edited

Proposed Changes

Additional Info

Checklist

codecov bot commented Feb 8, 2022 • edited

Codecov Report

magik6k commented Feb 8, 2022

TheDivic commented Feb 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brdji commented Feb 17, 2022

TheDivic commented Feb 8, 2022 •

edited

codecov bot commented Feb 8, 2022 •

edited