Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test Network Testing Framework #6489

Merged
merged 39 commits into from
Jun 26, 2020
Merged

Conversation

alexanderbez
Copy link
Contributor

@alexanderbez alexanderbez commented Jun 23, 2020

Description

Introduction of the testutil package. This package allows the creation of an entirely in-process testing cluster with fully operational Tendermint nodes constructed with SimApp. Each node has an RPC & API exposed. In addition, the network exposes a Local client that can be used to directly interface with Tendermint's RPC. The test network is entirely configurable. In the future, we should explore how to pass dynamic app constructors so other applications/projects can utilize this.

This is the foundation of #6423. With this framework, we'll be able to easily spin up and tear down networks for rapid integration testing and development. Once we refactor our CLI handling, we can entirely ditch all the current "fixtures".

I've included a "liveness" test as an example and replacement of the ad-hoc script we have. I've also included an integration test suite for x/bank API handlers.

/cc @aaronc @jackzampolin @anilcse @marbar3778


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

Comment on lines -223 to +236
if err := apiSrv.Start(config); err != nil {
errCh := make(chan error)

go func() {
if err := apiSrv.Start(config); err != nil {
errCh <- err
}
}()

select {
case err := <-errCh:
return err
case <-time.After(5 * time.Second): // assume server started successfully
Copy link
Contributor Author

@alexanderbez alexanderbez Jun 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not realize Tendermint HTTP client actually blocks -- this fixes that so we can properly cleanup below.

@codecov
Copy link

codecov bot commented Jun 23, 2020

Codecov Report

Merging #6489 into master will increase coverage by 4.51%.
The diff coverage is 85.29%.

@@            Coverage Diff             @@
##           master    #6489      +/-   ##
==========================================
+ Coverage   52.17%   56.68%   +4.51%     
==========================================
  Files         199      478     +279     
  Lines       12155    28777   +16622     
==========================================
+ Hits         6342    16313    +9971     
- Misses       5433    11306    +5873     
- Partials      380     1158     +778     

@alexanderbez alexanderbez mentioned this pull request Jun 23, 2020
19 tasks
server/api/server.go Outdated Show resolved Hide resolved
testutil/network.go Show resolved Hide resolved
testutil/network.go Show resolved Hide resolved
cdc.MustUnmarshalJSON(cfg.GenesisState[authtypes.ModuleName], &authGenState)

authGenState.Accounts = genAccounts
cfg.GenesisState[authtypes.ModuleName] = cdc.MustMarshalJSON(authGenState)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto cdc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment.

@alexanderbez
Copy link
Contributor Author

Failures are due to my apparent misunderstanding of Tendermint's multi-process capabilities -- specifically the RPC. It really is only meant to run in a single process, so starting multiple RPC clients is not going to work :-/

The only stopgap I can think of is to only allow one use of a network at a time and within a network, only one single validator's RPC server/client is started (which is OK).

This might be an ok tradeoff for now.

@alexanderbez
Copy link
Contributor Author

alexanderbez commented Jun 24, 2020

Still getting race conditions in Tendermint :-/

==================
WARNING: DATA RACE
Read at 0x00c000e22b30 by goroutine 166:
  github.com/tendermint/tendermint/p2p.(*NetAddress).String()
      /Users/aleksbez/go/pkg/mod/github.com/tendermint/tendermint@v0.33.5/p2p/netaddress.go:168 +0x4e
  github.com/tendermint/tendermint/p2p.(*NetAddress).Equals()
      /Users/aleksbez/go/pkg/mod/github.com/tendermint/tendermint@v0.33.5/p2p/netaddress.go:145 +0x5d
  github.com/tendermint/tendermint/p2p.(*Switch).IsPeerPersistent()
      /Users/aleksbez/go/pkg/mod/github.com/tendermint/tendermint@v0.33.5/p2p/switch.go:589 +0xbd
  github.com/tendermint/tendermint/p2p.(*Switch).IsPeerPersistent-fm()
      /Users/aleksbez/go/pkg/mod/github.com/tendermint/tendermint@v0.33.5/p2p/switch.go:587 +0x4b
  github.com/tendermint/tendermint/p2p.(*MultiplexTransport).wrapPeer()
      /Users/aleksbez/go/pkg/mod/github.com/tendermint/tendermint@v0.33.5/p2p/transport.go:491 +0x357
  github.com/tendermint/tendermint/p2p.(*MultiplexTransport).Accept()
      /Users/aleksbez/go/pkg/mod/github.com/tendermint/tendermint@v0.33.5/p2p/transport.go:200 +0x2f1
  github.com/tendermint/tendermint/p2p.(*Switch).acceptRoutine()
      /Users/aleksbez/go/pkg/mod/github.com/tendermint/tendermint@v0.33.5/p2p/switch.go:598 +0x2a1

Previous write at 0x00c000e22b30 by goroutine 168:
  github.com/tendermint/tendermint/p2p.(*NetAddress).String()
      /Users/aleksbez/go/pkg/mod/github.com/tendermint/tendermint@v0.33.5/p2p/netaddress.go:173 +0xfb
  github.com/tendermint/tendermint/p2p.(*NetAddress).Equals()
      /Users/aleksbez/go/pkg/mod/github.com/tendermint/tendermint@v0.33.5/p2p/netaddress.go:145 +0x5d
  github.com/tendermint/tendermint/p2p.(*Switch).IsPeerPersistent()
      /Users/aleksbez/go/pkg/mod/github.com/tendermint/tendermint@v0.33.5/p2p/switch.go:589 +0xbd
  github.com/tendermint/tendermint/p2p.(*Switch).IsPeerPersistent-fm()
      /Users/aleksbez/go/pkg/mod/github.com/tendermint/tendermint@v0.33.5/p2p/switch.go:587 +0x4b
  github.com/tendermint/tendermint/p2p.(*MultiplexTransport).wrapPeer()
      /Users/aleksbez/go/pkg/mod/github.com/tendermint/tendermint@v0.33.5/p2p/transport.go:487 +0x72
  github.com/tendermint/tendermint/p2p.(*MultiplexTransport).Dial()
      /Users/aleksbez/go/pkg/mod/github.com/tendermint/tendermint@v0.33.5/p2p/transport.go:228 +0x2aa
  github.com/tendermint/tendermint/p2p.(*Switch).addOutboundPeerWithConfig()
      /Users/aleksbez/go/pkg/mod/github.com/tendermint/tendermint@v0.33.5/p2p/switch.go:701 +0x45e
  github.com/tendermint/tendermint/p2p.(*Switch).DialPeerWithAddress()
      /Users/aleksbez/go/pkg/mod/github.com/tendermint/tendermint@v0.33.5/p2p/switch.go:537 +0x1b5
  github.com/tendermint/tendermint/p2p.(*Switch).dialPeersAsync.func1()
      /Users/aleksbez/go/pkg/mod/github.com/tendermint/tendermint@v0.33.5/p2p/switch.go:512 +0x2b3

Goroutine 166 (running) created at:
  github.com/tendermint/tendermint/p2p.(*Switch).OnStart()
      /Users/aleksbez/go/pkg/mod/github.com/tendermint/tendermint@v0.33.5/p2p/switch.go:234 +0x23a
  github.com/tendermint/tendermint/libs/service.(*BaseService).Start()
      /Users/aleksbez/go/pkg/mod/github.com/tendermint/tendermint@v0.33.5/libs/service/service.go:140 +0x504
  github.com/tendermint/tendermint/node.(*Node).OnStart()
      /Users/aleksbez/go/pkg/mod/github.com/tendermint/tendermint@v0.33.5/node/node.go:783 +0x605
  github.com/tendermint/tendermint/libs/service.(*BaseService).Start()
      /Users/aleksbez/go/pkg/mod/github.com/tendermint/tendermint@v0.33.5/libs/service/service.go:140 +0x504
  github.com/cosmos/cosmos-sdk/testutil.startInProcess()
      /Users/aleksbez/code/cosmos/cosmos-sdk/testutil/util.go:63 +0xa3b

I would imagine each node has its own unique address book (set of *NetAdress), so I'm not sure what's going on...

Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @alexanderbez ! Looks like a lot of heavy lifting.

I would like to have the configurability to use another app besides simapp for this functionality. I think that would basically mean having:

  • an app constructor on Config, and
  • test suites that have a constructor which takes a Config object

testutil/doc.go Outdated Show resolved Hide resolved
testutil/doc.go Outdated Show resolved Hide resolved
testutil/doc.go Outdated Show resolved Hide resolved
testutil/network.go Show resolved Hide resolved
testutil/util.go Outdated Show resolved Hide resolved
alexanderbez and others added 3 commits June 25, 2020 19:49
Co-authored-by: Aaron Craelius <aaron@regen.network>
Co-authored-by: Aaron Craelius <aaron@regen.network>
Co-authored-by: Aaron Craelius <aaron@regen.network>
@alexanderbez
Copy link
Contributor Author

alexanderbez commented Jun 25, 2020

Thanks @aaronc.

  • an app constructor on Config, and

Done.

test suites that have a constructor which takes a Config object

I don't follow. Can you elaborate, please?

@alexanderbez alexanderbez requested a review from aaronc June 26, 2020 00:32
@alexanderbez
Copy link
Contributor Author

Bump @aaronc @AdityaSripal on a re-review & comment followup. I'd like to get this merged so I can start addressing the CLI testing refactor.

Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexanderbez I added some TODOs to #6423 to track the TxGenerator migration and also to address reusability of IntegrationTestSuites. This looks like a great start.

@aaronc
Copy link
Member

aaronc commented Jun 26, 2020

test suites that have a constructor which takes a Config object

I don't follow. Can you elaborate, please?

Basically we could have a NewIntegrationTestSuite(Config) constructor that allows for the IntegrationTestSuite to be used by different apps. We would also move IntegrationTestSuite to a helpers file so it can be used elsewhere. Then the default test runner code would look like:

	func TestIntegrationTestSuite(t *testing.T) {
 		suite.Run(t, new IntegrationTestSuite(testutil.DefaultConfig())
 	}

I added a TODO to #6423 about doing this in a separate PR.

@alexanderbez
Copy link
Contributor Author

IMHO that might be overkill.

@aaronc
Copy link
Member

aaronc commented Jun 26, 2020

IMHO that might be overkill.

What's overkill?

@alexanderbez
Copy link
Contributor Author

#6489 (comment)

@aaronc
Copy link
Member

aaronc commented Jun 26, 2020

#6489 (comment)

Why would that be overkill? Seems pretty simple and isn't one of the points to be able to reuse integration tests in gaia and other apps?

@alexanderbez
Copy link
Contributor Author

IntegrationTestSuite isn't meant to be used by other apps. Each package should define its own test suite. Doing what you're suggesting doesn't really buy us anything valuable.

@aaronc
Copy link
Member

aaronc commented Jun 26, 2020 via email

@alexanderbez
Copy link
Contributor Author

Mhhh, ok I'm still not really following what IntegrationTestSuite buys you than you can't just do directly. In any case, I'm not going to block this PR on this. Feel free to open up a follow-up PR with this ask and I'll be happy to review.

@alexanderbez alexanderbez dismissed AdityaSripal’s stale review June 26, 2020 16:30

Addressed all comments.

@alexanderbez alexanderbez merged commit 9bf3ff7 into master Jun 26, 2020
@alexanderbez alexanderbez deleted the bez/in-process-test-framework branch June 26, 2020 16:30
@aaronc
Copy link
Member

aaronc commented Jun 26, 2020

Mhhh, ok I'm still not really following what IntegrationTestSuite buys you than you can't just do directly. In any case, I'm not going to block this PR on this. Feel free to open up a follow-up PR with this ask and I'll be happy to review.

Yep, like I said it can be addressed later and I put a TODO in #6423.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. T: Tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants