Future Test Refactoring

[S1nus]

Follow-up from https://github.com/rollkit/rollkit/pull/963

• [x] Some test helpers are copy-pasted across multiple test files such as https://github.com/rollkit/rollkit/blob/main/da/test/da_test.go#L247. These can be moved to a common rollkit/test package. There are other functions like this in node/full_client_test.go, such as getRandomTx(...) and getRandomBytes(...), and getRPC(...) in rpc/json/service_test.go.
• [ ] Create a constant for blocktime for usage in testing
• [ ] streamline this common pattern with a new helper:

fullNode, _ := createNode(ctx, 0, false, true, false, keys, t)
	lightNode, _ := createNode(ctx, 1, false, true, true, keys, t)
	fullNode.(*FullNode).dalc = dalc
	fullNode.(*FullNode).blockManager.SetDALC(dalc)

[AryanGodara]

Hi @Manav-Aggarwal & @S1nus, ser this looks like a big task and somewhat related to #1320, as well as other issues. I have past experience writing tests for distributed systems in Golang. I would like to contribute to this issue. Could you please direct me on where to start? I'd like to pick this up piece by piece and start working on it.

[Manav-Aggarwal]

Hey @AryanGodara, thanks for offering to contribute on this! I revisited this and seems like a lot of the first part of this issue was done in https://github.com/rollkit/rollkit/pull/1306.

For the first step, I would find any other places where test helpers are copy-pasted and can be moved to a utils file as done in the above PR.

Create a constant for blocktime for usage in testing Regarding this, I don't think this relevant anymore, tagging @gupadhyaya to confirm.

streamline this common pattern with a new helper this should be pretty apparent from the description itself, you'll see that pattern in a bunch of places in the codebase so if you find a way to refactor it into a helper that can used cleanly across the codebase, that'd be great as a step too.

I have untagged this as part of vA since the majority of the de-duplication seems to have been covered by https://github.com/rollkit/rollkit/pull/1306.

[AryanGodara]

Hey @AryanGodara, thanks for offering to contribute on this! I revisited this and seems like a lot of the first part of this issue was done in https://github.com/rollkit/rollkit/pull/1306.

For the first step, I would find any other places where test helpers are copy-pasted and can be moved to a utils file as done in the above PR.

Create a constant for blocktime for usage in testing

Regarding this, I don't think this relevant anymore, tagging @gupadhyaya to confirm.

streamline this common pattern with a new helper

this should be pretty apparent from the description itself, you'll see that pattern in a bunch of places in the codebase so if you find a way to refactor it into a helper that can used cleanly across the codebase, that'd be great as a step too.

I have untagged this as part of vA since the majority of the de-duplication seems to have been covered by https://github.com/rollkit/rollkit/pull/1306.

Thanks for the prompt reply @Manav-Aggarwal !! I'll get to this asap, and open a draft PR when I make progress?

[AryanGodara]

@Manav-Aggarwal , I've been looking into these three functions

// GetRandomBlock returns a block with random data
func GetRandomBlock(height uint64, nTxs int) *Block {
	block, _ := GetRandomBlockWithKey(height, nTxs, nil)
	return block
}

func GetRandomBlockWithProposer(height uint64, nTxs int, proposerAddr []byte) *Block {
	block, _ := GetRandomBlockWithKey(height, nTxs, nil)
	block.SignedHeader.ProposerAddress = proposerAddr

	return block
}

// GetRandomBlockWithKey returns a block with random data and a signing key
func GetRandomBlockWithKey(height uint64, nTxs int, privKey ed25519.PrivKey) (*Block, ed25519.PrivKey) {...}

Each just calls GetRandomBlockWithKey We can just have one GetRandomBlock, and call it as we wish; even make the signedKey an optional variable, or simply pass nil when we don't want to pass in a key explicitly.

All in all, it seems that this is a bit redundant code (to have multiple functions doing the same thing), imo.

[AryanGodara]

Other than this, I went through your previous discussions on assert.New vs assert.Method with testing.t as parameter; I, too, believe the second approach is better and something I've always used as well. So, I'll update those in the same PR as well, if it's okay [for now, I've checked that there are no remaining functions where assert or require are directly being passed as parameters]

[AryanGodara]

So, I saw that you had already done a great optimization since Go doesn't allow default values, and using the (...) variadic pattern isn't viable here. So, I looked up alternatives, and thought we could use a config struct, with Functional Options wrapping. So, it'll be just one function, that looks like this:-

// BlockOption is a functional option for creating a block.
type BlockOption func(*BlockOptions)

// BlockOptions is a set of options for creating a block.
type BlockOptions struct {
	privKey      ed25519.PrivKey
	proposerAddr []byte
}

// WithPrivKey sets a private key for signing the block.
func WithPrivKey(privKey ed25519.PrivKey) BlockOption {
	return func(opts *BlockOptions) {
		opts.privKey = privKey
	}
}

// WithProposerAddress sets the proposer address for the block.
func WithProposerAddress(proposerAddr []byte) BlockOption {
	return func(opts *BlockOptions) {
		opts.proposerAddr = proposerAddr
	}
}

// GetRandomBlock returns a block with random data and the given height
func GetRandomBlock(height uint64, nTxs int, options ...BlockOption) (*Block, ed25519.PrivKey) {
	blockOpts := BlockOptions{}
	for _, option := range options {
		option(&blockOpts)
	}

	block := getBlockDataWith(nTxs)
	dataHash, err := block.Data.Hash()
	if err != nil {
		panic(err)
	}

	signedHeader, privKey, err := GetRandomSignedHeaderWith(height, dataHash, blockOpts.privKey)
	if err != nil {
		panic(err)
	}
	block.SignedHeader = *signedHeader

	if blockOpts.proposerAddr != nil {
		block.SignedHeader.ProposerAddress = blockOpts.proposerAddr
	}

	return block, privKey
}

[AryanGodara]

Hi @Manav-Aggarwal! According to this comment, do we still want the require.New() to be removed from all tests and shift to require.Method() approach (same with assert). Because rn, it's a mix of both these approaches, in different functions, and maybe we can stick to one approach?

[Manav-Aggarwal]

Done in https://github.com/rollkit/rollkit/pull/1545