add ability to mock zcashd rpc calls for testing, add tests by LarryRuane · Pull Request #164 · zcash/lightwalletd

LarryRuane · 2020-01-06T17:49:57Z

Fixes #146. Small block caching test improvement, but also make the zcashd RPC calls mockable, so tests can generate fake replies, to increase test coverage.

LarryRuane

The main changes here are to allow calls to zcashd RPCs to be stubbed out. There are a few more instances to do, but this is a start.

LarryRuane · 2020-01-17T18:16:29Z

cmd/server/main.go

+		defer output.Close()
+		logger.SetOutput(output)
+		logger.SetFormatter(&logrus.JSONFormatter{})
+	}


This is moved, makes sense to open the log file as soon as possible. Slight change (improvement) in behavior -- if you specify -log-file '', the process will log to stdout. (Without these changes, it's not possible to log to stdout.)

LarryRuane · 2020-01-17T18:18:17Z

common/cache.go


 	// Detect reorg, ingestor needs to handle it
-	if height > c.firstBlock && !bytes.Equal(block.PrevHash, c.m[height-1].hash) {
+	if block != nil && height > c.firstBlock && !bytes.Equal(block.PrevHash, c.m[height-1].hash) {


The new nil check allows the unit test to force an error below, on the call to proto.Marshal(). I don't love modifying production code just for testing, but I think in this case it's okay, it really is caught below. (This is the only instance of that.)

It's worth adding a comment in the code to explain this.

common/common.go

LarryRuane · 2020-01-17T18:20:59Z

common/common.go

 	retryCount := 0
-	for {
-		block, err := getBlockFromRPC(rpcClient, height)
+	for i := 0; rep == 0 || i < rep; i++ {


The rep variable prevents this function from infinite-looping during unit testing (it will, correctly, infinite loop during production).

common/common_test.go

defuse · 2020-01-29T19:13:28Z

Interesting, github says this PR was closed but it was actually merged. I guess it does that when the branch gets merged differently to how GitHub expects?

LarryRuane · 2020-01-29T21:41:02Z

I closed this by mistake, reopening

LarryRuane · 2020-01-29T21:53:28Z

Makefile

 # Lint golang files
 lint:
-	@golint -set_exit_status
+	golint -set_exit_status


I find it much more helpful to see what's actually happening

LarryRuane · 2020-01-29T21:54:05Z

Makefile

 # Run unittests
 test:
-	@go test -v -coverprofile=coverage.txt -covermode=atomic ./...
+	go test -v ./...


there is no need for atomic, and there's a separate make target for coverage

LarryRuane · 2020-01-29T21:54:53Z

Makefile

 coverage:
-	@go test -coverprofile=coverage.out -covermode=atomic ./...
+	go test -coverprofile=coverage.out ./...
+	sed -i '/\.pb\.go/d' coverage.out


the .pb. files are generated (protobuf), and their code coverage is not relevant

LarryRuane · 2020-01-29T21:57:51Z

cmd/server/main.go

 	}

-	log = logger.WithFields(logrus.Fields{
+	common.Log = logger.WithFields(logrus.Fields{


There is no reason the logging object can't be a global variable, and doing that simplifies many of the function signatures, and makes unit tests easier to write.

LarryRuane · 2020-01-29T22:01:31Z

common/cache.go

+type blockCacheEntry struct {
 	data []byte
 	hash []byte
 }


Identifiers that start with a capital letter are exported, but this type definition can remain private.

LarryRuane · 2020-01-29T22:19:51Z

docker/gen_cert.sh

 # apt-get update && apt-get install -y openssl

-openssl req -x509 -nodes -newkey rsa:2048 -keyout ./cert.key -out ./cert.pem -days 3650 -subj "/C=US/ST=CA/O=MyOrg, Inc./CN=lightwalletd.testnet.local" -reqexts SAN -config <(cat /etc/ssl/openssl.cnf <(printf "\n[SAN]\nsubjectAltName=DNS:lightwalletd.testnet.local,DNS:127.0.0.1,DNS:localhost"))
+openssl req -x509 -nodes -newkey rsa:2048 -keyout ./cert.key -out ./cert.pem -days 3650 -subj "/C=US/ST=CA/O=MyOrg, Inc./CN=lightwalletd.testnet.local" -reqexts SAN -config <(cat /etc/ssl/openssl.cnf <(printf "\n[SAN]\nsubjectAltName=DNS:lightwalletd.testnet.local,DNS:127.0.0.1,DNS:localhost"))


missing newline (confuses some tools)

LarryRuane · 2020-01-29T22:24:26Z

frontend/frontend_test.go

+	"t123456789012345678901234567890123*",  // invalid "*"
+	"s1234567890123456789012345678901234",  // doesn't start with "t"
+	" t1234567890123456789012345678901234", // extra stuff before
+	"t1234567890123456789012345678901234 ", // extra stuff after


This test case addresses adityapk00#4

A test case for the newline thing would be "\nt1234567890123456789012345678901234".

LarryRuane · 2020-01-29T22:26:00Z

frontend/rpc_client.go

 	rpcaddr := cfg.Section("").Key("rpcbind").String()
+	if rpcaddr == "" {
+		rpcaddr = "127.0.0.1"
+	}


Should not be in this PR, but I ran into this while testing, so...

LarryRuane · 2020-01-29T22:28:03Z

frontend/service.go

-		s.log.Errorf("Got error: %s", rpcErr.Error())
-		return nil
+		common.Log.Errorf("Got error: %s", rpcErr.Error())
+		return err


This (and a few similar cases below) was just broken, nil means no error by convention.

Were these maybe intentional for some reason, like to hide the specific error message from the client or something? (I don't think so, it looks like it was just broken)

LarryRuane · 2020-01-29T22:31:20Z

parser/transaction_test.go

-
-		rawTxData[count] = txData
-		count++
+		rawTxData = append(rawTxData, txData)


This is just a slight simplification, no functional change

LarryRuane

Interesting, github says this PR was closed but it was actually merged. I guess it does that when the branch gets merged differently to how GitHub expects?

I think GitHub noticed that the tip of this branch was equal to the tip of the target branch (master), so it concluded that the changes were merged... when in reality, there were no changes (because of my mistake of force-pushing my branch when it exactly matched master). And because the changes were "merged", it figured that the PR can be closed! Anyway, it's back in the correct state now.

defuse

Tested ACK. This is some awesome testing!! I left some comments, but none of them are blocking.

defuse · 2020-01-31T19:25:05Z

common/cache.go


 	// Detect reorg, ingestor needs to handle it
-	if height > c.firstBlock && !bytes.Equal(block.PrevHash, c.m[height-1].hash) {
+	if block != nil && height > c.firstBlock && !bytes.Equal(block.PrevHash, c.m[height-1].hash) {


It's worth adding a comment in the code to explain this.

defuse · 2020-01-31T19:32:57Z

frontend/service.go

-		s.log.Errorf("Got error: %s", rpcErr.Error())
-		return nil
+		common.Log.Errorf("Got error: %s", rpcErr.Error())
+		return err


Were these maybe intentional for some reason, like to hide the specific error message from the client or something? (I don't think so, it looks like it was just broken)

defuse · 2020-01-31T19:38:49Z

frontend/frontend_test.go

+	"t123456789012345678901234567890123*",  // invalid "*"
+	"s1234567890123456789012345678901234",  // doesn't start with "t"
+	" t1234567890123456789012345678901234", // extra stuff before
+	"t1234567890123456789012345678901234 ", // extra stuff after


A test case for the newline thing would be "\nt1234567890123456789012345678901234".

defuse · 2020-01-31T19:44:28Z

common/common_test.go

+func TestGenerateCerts(t *testing.T) {
+	if GenerateCerts() == nil {
+		t.Fatal("GenerateCerts returned nil")
+	}


I opened #181 so that we're less likely to mistakenly think GenerateCerts is fully tested when we look at the coverage (it'd be obvious if this code was broken because the certs wouldn't work so this is fine for now).

LarryRuane · 2020-01-31T23:36:38Z

Force-pushed @defuse's review comment fixes (thanks!), merging now.

LarryRuane self-assigned this Jan 6, 2020

LarryRuane mentioned this pull request Jan 8, 2020

Reorg-Handling Tests & lightwalletd<->client Reorg-Handling Contract #52

Closed

LarryRuane force-pushed the more-test-coverage branch from a260134 to 3083d45 Compare January 17, 2020 18:09

LarryRuane requested a review from defuse January 17, 2020 18:10

LarryRuane force-pushed the more-test-coverage branch from 3083d45 to aeadd48 Compare January 17, 2020 18:13

LarryRuane commented Jan 17, 2020

View reviewed changes

LarryRuane force-pushed the more-test-coverage branch from aeadd48 to 1dbec79 Compare January 27, 2020 03:50

LarryRuane changed the title ~~[WIP] add ability to mock zcashd rpc calls for testing~~ add ability to mock zcashd rpc calls for testing, add tests Jan 27, 2020

LarryRuane closed this Jan 27, 2020

LarryRuane force-pushed the more-test-coverage branch from 1dbec79 to 59170d4 Compare January 27, 2020 04:01

LarryRuane reopened this Jan 29, 2020

LarryRuane commented Jan 29, 2020

View reviewed changes

defuse approved these changes Jan 31, 2020

View reviewed changes

test improvements, and minor cleanups

eca8f66

LarryRuane force-pushed the more-test-coverage branch from 3fab586 to eca8f66 Compare January 31, 2020 22:17

LarryRuane merged commit a4f9688 into master Jan 31, 2020

LarryRuane deleted the more-test-coverage branch January 31, 2020 23:36

LarryRuane mentioned this pull request Feb 3, 2020

Unit Tests Using Mocking Layer #163

Closed

1 task

Comments

Conversation

LarryRuane commented Jan 6, 2020

Uh oh!

LarryRuane left a comment

Choose a reason for hiding this comment

Uh oh!

LarryRuane Jan 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LarryRuane Jan 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

defuse commented Jan 29, 2020

Uh oh!

LarryRuane commented Jan 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LarryRuane left a comment

Choose a reason for hiding this comment

Uh oh!

defuse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LarryRuane commented Jan 31, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

LarryRuane Jan 17, 2020 •

edited

Loading

LarryRuane Jan 17, 2020 •

edited

Loading