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

revamped integration test kit (aka. Operation Sparks Joy) #6329

Merged
merged 102 commits into from
Jun 28, 2021

Conversation

raulk
Copy link
Member

@raulk raulk commented May 25, 2021

Gradual migration from kit1 to kit2

We will be migrating integration tests gradually. For that reason, this PR maintains the old test kit alongside the new one. So far, we have migrated api_test.go and deals_test.go. The remaining tests will be migrated as part of #6462.


Description

This PR introduces a revamped and cleaner testkit that (hopefully) will make writing integration tests enjoyable again(TM).

The API is under package itests/kit. It revolves around the concept of an Ensemble: a collection of full nodes and miners that together form the network for the test.

Create a new ensemble with:

ens := kit.NewEnsemble()

Create full nodes and miners:

var full TestFullNode
var miner TestMiner
ens.FullNode(&full, opts...)       // populates a full node
ens.Miner(&miner, &full, opts...)  // populates a miner, using the full node as its chain daemon

You can pass functional options to set initial balances, presealed sectors, owner keys, etc.

After the initial nodes are added, call ens.Start() to forge the genesis and start the network.

You can then continue to add more nodes, but you must always follow with ens.Start() to activate the new nodes.

There are methods to interconnect all nodes (InterconnectNodes) and begin the mining process (BeginMining).

The API is chainable, so it's possible to do a lot in a very succinct way ;-)

kit.NewEnsemble().FullNode(&full).Miner(&miner, &full).Start().BeginMining()

There is some sugar for preset ensembles:

kit.EnsembleMinimal() // gives you a one node, one miner setup
kit.EnsembleTwo()     // gives you a two node, one miner setup

TODO

  • Migrate all integration tests (currently prevented from compiling).
    • api_test.go
    • deals_test.go
    • [ ] batch_deal_test.go
    • [ ] ccupgrade_test.go
    • [ ] cli_test.go
    • [ ] deadlines_test.go
    • [ ] gateway_test.go
    • [ ] multisig_test.go
    • [ ] paych_api_test.go
    • [ ] paych_cli_test.go
    • [ ] sdr_upgrade_test.go
    • [ ] sector_pledge_test.go
    • [ ] sector_terminate_test.go
    • [ ] tape_test.go
    • [ ] wdpost_dispute_test.go
    • [ ] wdpost_test.go
    • Tests will be migrated gradually from kit1 to kit2.
  • Add ability to pass in arbitrary constructor parameters via a functional option: NodeOpts(opts...)
  • Add godocs.
  • Cleanup and bugfixing.

Flaky tests to debug

  • TestConcurrentDeals, usually when n=8
  • TestPublishDealsBatching

Still need to migrate all integration tests, add godocs,
and probably zap bugs.
@raulk raulk changed the title initial version of the new itest kit. revamped integration test kit May 25, 2021
@raulk raulk added the team/ignite Issues and PRs being tracked by Team Ignite at Protocol Labs label Jun 9, 2021
Base automatically changed from raulk/itests to master June 10, 2021 10:33
Copy link
Contributor

@dirkmc dirkmc left a comment

Choose a reason for hiding this comment

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

A few optional suggestions but looks great 👍

if len(p) != 0 {
t.Error("Node 0 has a peer")
}
one, two, _, ens := kit2.EnsembleTwoOne(t, ts.opts...)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: suggest renaming these ensemble methods to EnsembleTwoFullOneMiner etc for clarity

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense, but I think that's too long. We can find a convention like:

kit2.Ensemble2M1F()

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd err on the side of being verbose - more of a nitpick though, we can address in a later PR

itests/deals_test.go Outdated Show resolved Hide resolved
itests/kit2/ensemble.go Outdated Show resolved Hide resolved
_ = logging.SetLogLevel("*", "INFO")

policy.SetConsensusMinerMinPower(abi.NewStoragePower(2048))
policy.SetSupportedProofTypes(abi.RegisteredSealProof_StackedDrg2KiBV1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest adding 8MB proof types as well so we can run integration tests with 2k or 8MB sector sizes

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense. Feel free to push the change directly!

@raulk raulk marked this pull request as ready for review June 25, 2021 21:34
@raulk
Copy link
Member Author

raulk commented Jun 25, 2021

Readying for review since the ccupgrade test fails on master anyway.

Copy link
Contributor

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

Looks good, much better than what we had, but we should undo the things which were done to the ccupgarde test, as on master it is flake, but here it seems to be completely broken, not even being able to setup genesis -> https://app.circleci.com/pipelines/github/filecoin-project/lotus/15278/workflows/af54db65-6468-4207-a70e-b47023166b6f/jobs/180494

@raulk raulk force-pushed the raulk/itests-refactor-kit branch from 51a1542 to 6a48fbb Compare June 28, 2021 10:51
@raulk
Copy link
Member Author

raulk commented Jun 28, 2021

@magik6k reverted the commits where I was fiddling with genesis balances to try to game the CE parameters to make this test pass.

@magik6k magik6k merged commit cefd140 into master Jun 28, 2021
@magik6k magik6k deleted the raulk/itests-refactor-kit branch June 28, 2021 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team/ignite Issues and PRs being tracked by Team Ignite at Protocol Labs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants