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.

Sign up for GitHub

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

Jump to bottom

chore: pass knuu as an argument to the testnet.New #3952

Merged
merged 18 commits into from
Oct 14, 2024
Merged

chore: pass knuu as an argument to the testnet.New #3952

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
3b86471
chore: pass knuu as an argument to the testnet.New
mojtaba-esk Oct 8, 2024
c0589bd
Merge branch 'main' into mojtaba/pass-knuu-as-an-argument-to-the-test…
mojtaba-esk Oct 8, 2024
53d2601
chore: update benchmark test to use tetsnet.Options
mojtaba-esk Oct 9, 2024
0a08b11
chore: log message cleanup
mojtaba-esk Oct 9, 2024
80b2136
Merge branch 'main' into mojtaba/pass-knuu-as-an-argument-to-the-test…
mojtaba-esk Oct 9, 2024
a51789c
chore: fix linter complains
mojtaba-esk Oct 9, 2024
6d00b69
chore: extract timeFormat constant
mojtaba-esk Oct 9, 2024
c17e943
chore: extract timeFormat constant
mojtaba-esk Oct 9, 2024
064e27a
Merge branch 'main' into mojtaba/pass-knuu-as-an-argument-to-the-test…
mojtaba-esk Oct 10, 2024
3d3d3d6
chore: resolve conflicts
mojtaba-esk Oct 10, 2024
24ed7e9
chore: a simple validation added for the options
mojtaba-esk Oct 10, 2024
c7ebeb6
chore: extract knuu from options
mojtaba-esk Oct 11, 2024
57e1f55
Merge branch 'main' into mojtaba/pass-knuu-as-an-argument-to-the-test…
mojtaba-esk Oct 11, 2024
f7d9104
chore: removed knuu from tetsnet options
mojtaba-esk Oct 11, 2024
58b775f
Merge branch 'main' into mojtaba/pass-knuu-as-an-argument-to-the-test…
mojtaba-esk Oct 14, 2024
8a6401c
Update test/e2e/major_upgrade_v3.go
mojtaba-esk Oct 14, 2024
26af4c9
Update test/e2e/benchmark/benchmark.go
mojtaba-esk Oct 14, 2024
2e83e48
Apply suggestions from code review
mojtaba-esk Oct 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 23 additions & 4 deletions test/e2e/benchmark/benchmark.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"github.com/celestiaorg/celestia-app/v3/pkg/appconsts"
"github.com/celestiaorg/celestia-app/v3/test/e2e/testnet"
"github.com/celestiaorg/celestia-app/v3/test/util/testnode"
"github.com/celestiaorg/knuu/pkg/knuu"
)

type BenchmarkTest struct {
Expand All @@ -20,14 +21,32 @@ type BenchmarkTest struct {
}

func NewBenchmarkTest(name string, manifest *Manifest) (*BenchmarkTest, error) {
// create a new testnet
testNet, err := testnet.New(context.Background(), name, seed,
testnet.GetGrafanaInfoFromEnvVar(), manifest.ChainID,
manifest.GetGenesisModifiers()...)
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
mojtaba-esk marked this conversation as resolved.
Show resolved Hide resolved

identifier := fmt.Sprintf("%s_%s", name, time.Now().Format("20060102_150405"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

[optional] this could extract a constant for the time format and reuse that in other places

Copy link
Member Author

Choose a reason for hiding this comment

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

done

kn, err := knuu.New(ctx, knuu.Options{
Scope: identifier,
ProxyEnabled: true,
})
if err != nil {
return nil, err
}

// context.Background() is used to allow the stopSignal to be functioning even after this function returns
mojtaba-esk marked this conversation as resolved.
Show resolved Hide resolved
kn.HandleStopSignal(context.Background())

log.Printf("Knuu initialized with scope %s", kn.Scope)
Copy link
Collaborator

Choose a reason for hiding this comment

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

[nit] I would move this log statement immediately beneath the Knuu initialization command (and error handling):

	kn, err := knuu.New(ctx, knuu.Options{
		Scope:        identifier,
		ProxyEnabled: true,
	})
	if err != nil {
		return nil, err
	}
	log.Printf("Knuu initialized with scope %s", kn.Scope)


testNet, err := testnet.New(testnet.Options{
Seed: seed,
Grafana: testnet.GetGrafanaInfoFromEnvVar(),
ChainID: manifest.ChainID,
GenesisModifiers: manifest.GetGenesisModifiers(),
Knuu: kn,
})
testnet.NoError("failed to create testnet", err)

testNet.SetConsensusParams(manifest.GetConsensusParams())
return &BenchmarkTest{Testnet: testNet, manifest: manifest}, nil
}
Expand Down
23 changes: 21 additions & 2 deletions test/e2e/major_upgrade_v2.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,28 +12,47 @@ import (
v1 "github.com/celestiaorg/celestia-app/v3/pkg/appconsts/v1"
v2 "github.com/celestiaorg/celestia-app/v3/pkg/appconsts/v2"
"github.com/celestiaorg/celestia-app/v3/test/e2e/testnet"
"github.com/celestiaorg/knuu/pkg/knuu"
"github.com/tendermint/tendermint/rpc/client/http"
)

func MajorUpgradeToV2(logger *log.Logger) error {
var (
testName = "MajorUpgradeToV2"
numNodes = 4
upgradeHeight = int64(10)
)
mojtaba-esk marked this conversation as resolved.
Show resolved Hide resolved

ctx, cancel := context.WithCancel(context.Background())
defer cancel()

identifier := fmt.Sprintf("%s_%s", testName, time.Now().Format("20060102_150405"))
kn, err := knuu.New(ctx, knuu.Options{
Scope: identifier,
ProxyEnabled: true,
// if the tests timeout, pass the timeout option
// Timeout: 120 * time.Minute,
})
testnet.NoError("failed to initialize Knuu", err)

kn.HandleStopSignal(ctx)
logger.Printf("Knuu initialized with scope %s", kn.Scope)
Copy link
Collaborator

Choose a reason for hiding this comment

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

[nit] same nit about moving this immediately after knuu initialization.

Copy link
Member Author

Choose a reason for hiding this comment

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

it is after knuu initialization. knuu.New() does the initialization and if it does not fail (error check), then we handle the stop signal.

or you mean removing that empty line?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean moving the log statement.

	kn, err := knuu.New(ctx, knuu.Options{
		Scope:        scope,
		ProxyEnabled: true,
	})
	testnet.NoError("failed to initialize Knuu", err)
+	logger.Printf("Knuu initialized with scope %s", kn.Scope)

	kn.HandleStopSignal(ctx)
-	logger.Printf("Knuu initialized with scope %s", kn.Scope)


logger.Println("Creating testnet")
testNet, err := testnet.New(ctx, "MajorUpgradeToV2", seed, nil, "test")
testNet, err := testnet.New(testnet.Options{
Seed: seed,
Grafana: nil,
ChainID: "test",
Knuu: kn,
})
mojtaba-esk marked this conversation as resolved.
Show resolved Hide resolved
testnet.NoError("failed to create testnet", err)

defer testNet.Cleanup(ctx)

latestVersion, err := testnet.GetLatestVersion()
testnet.NoError("failed to get latest version", err)

logger.Println("Running major upgrade to v2 test", "version", latestVersion)
logger.Printf("Running %s test with version %s", testName, latestVersion)

testNet.SetConsensusParams(app.DefaultInitialConsensusParams())

Expand Down
27 changes: 24 additions & 3 deletions test/e2e/minor_version_compatibility.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,14 @@ import (
v1 "github.com/celestiaorg/celestia-app/v3/pkg/appconsts/v1"
v2 "github.com/celestiaorg/celestia-app/v3/pkg/appconsts/v2"
"github.com/celestiaorg/celestia-app/v3/test/e2e/testnet"
"github.com/celestiaorg/knuu/pkg/knuu"
)

func MinorVersionCompatibility(logger *log.Logger) error {
const numNodes = 4
const (
testName = "MinorVersionCompatibility"
numNodes = 4
)
Comment on lines +22 to +25
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const (
testName = "MinorVersionCompatibility"
numNodes = 4
)
testName := "MinorVersionCompatibility"
numNodes = 4


versionStr, err := getAllVersions()
testnet.NoError("failed to get versions", err)
Expand All @@ -30,12 +34,29 @@ func MinorVersionCompatibility(logger *log.Logger) error {
logger.Fatal("no versions to test")
}
r := rand.New(rand.NewSource(seed))
logger.Println("Running minor version compatibility test", "versions", versions)
logger.Printf("Running %s test with versions %s", testName, versions)

ctx, cancel := context.WithCancel(context.Background())
defer cancel()

testNet, err := testnet.New(ctx, "MinorVersionCompatibility", seed, nil, "test")
identifier := fmt.Sprintf("%s_%s", testName, time.Now().Format("20060102_150405"))
kn, err := knuu.New(ctx, knuu.Options{
Scope: identifier,
ProxyEnabled: true,
// if the tests timeout, pass the timeout option
// Timeout: 120 * time.Minute,
})
mojtaba-esk marked this conversation as resolved.
Show resolved Hide resolved
testnet.NoError("failed to initialize Knuu", err)
mojtaba-esk marked this conversation as resolved.
Show resolved Hide resolved

kn.HandleStopSignal(ctx)
logger.Printf("Knuu initialized with scope %s", kn.Scope)

testNet, err := testnet.New(testnet.Options{
Seed: seed,
Grafana: nil,
ChainID: "test",
Knuu: kn,
})
testnet.NoError("failed to create testnet", err)

defer testNet.Cleanup(ctx)
Expand Down
23 changes: 21 additions & 2 deletions test/e2e/simple.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,24 +9,43 @@ import (
"github.com/celestiaorg/celestia-app/v3/pkg/appconsts"
"github.com/celestiaorg/celestia-app/v3/test/e2e/testnet"
"github.com/celestiaorg/celestia-app/v3/test/util/testnode"
"github.com/celestiaorg/knuu/pkg/knuu"
)

// This test runs a simple testnet with 4 validators. It submits both MsgPayForBlobs
// and MsgSends over 30 seconds and then asserts that at least 10 transactions were
// committed.
func E2ESimple(logger *log.Logger) error {
const testName = "E2ESimple"

ctx, cancel := context.WithCancel(context.Background())
defer cancel()

testNet, err := testnet.New(ctx, "E2ESimple", seed, nil, "test")
identifier := fmt.Sprintf("%s_%s", testName, time.Now().Format("20060102_150405"))
kn, err := knuu.New(ctx, knuu.Options{
Scope: identifier,
ProxyEnabled: true,
// if the tests timeout, pass the timeout option
// Timeout: 120 * time.Minute,
})
testnet.NoError("failed to initialize Knuu", err)
kn.HandleStopSignal(ctx)
logger.Printf("Knuu initialized with scope %s", kn.Scope)

testNet, err := testnet.New(testnet.Options{
Seed: seed,
Grafana: nil,
ChainID: "test",
Knuu: kn,
})
testnet.NoError("failed to create testnet", err)

defer testNet.Cleanup(ctx)

latestVersion, err := testnet.GetLatestVersion()
testnet.NoError("failed to get latest version", err)

logger.Println("Running E2ESimple test", "version", latestVersion)
logger.Printf("Running %s test with version %s", testName, latestVersion)

logger.Println("Creating testnet validators")
testnet.NoError("failed to create genesis nodes",
Expand Down
43 changes: 13 additions & 30 deletions test/e2e/testnet/testnet.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,42 +30,25 @@ type Testnet struct {
knuu *knuu.Knuu
}

func New(ctx context.Context, name string, seed int64, grafana *GrafanaInfo, chainID string,
genesisModifiers ...genesis.Modifier) (
*Testnet, error,
) {
identifier := fmt.Sprintf("%s_%s", name, time.Now().Format("20060102_150405"))
kn, err := knuu.New(ctx, knuu.Options{
Scope: identifier,
ProxyEnabled: true,
// if the tests timeout, pass the timeout option
// Timeout: 120 * time.Minute,
})
if err != nil {
return nil, err
}

log.Info().
Str("scope", kn.Scope).
Str("TestName", name).
Msg("Knuu initialized")

kn.HandleStopSignal(ctx)
type Options struct {
Seed int64
Grafana *GrafanaInfo
ChainID string
GenesisModifiers []genesis.Modifier
Knuu *knuu.Knuu
}
mojtaba-esk marked this conversation as resolved.
Show resolved Hide resolved

func New(opts Options) (*Testnet, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I prefer the use of Options for arguments to a constructor which are in fact optional. All fields here can have working defaults except for the Knuu instance which is mandatory thus I'd prefer

Suggested change
func New(opts Options) (*Testnet, error) {
func New(knuu *knuu.Knuu, opts Options) (*Testnet, error) {

so now it's more explicit or better still if you go with

type Option func (*Testnet) 

func WithSeed(seed int64) Option {
    return func (t *Testnet) {
        t.seed = seed
    }
}

func New(knuu *knuu.Knuu, options Option...) *Testnet

Copy link
Member Author

Choose a reason for hiding this comment

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

yes the term "option" is kinda counter intuitive here. The main reason for using it to reduce the number of arguments to the constructor.
Function option could be another way of using it; it is subjective, but I think option alone is cleaner than than the function option way.
we could extract knuu out of that option thing. a question might arise here: do we have default values for the rest of options if we extract knuu out? if so, I will do that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Here's a guess at defaults for the options:

	Seed             int64 => 42
	Grafana          *GrafanaInfo => &GrafanaInfo{}
	ChainID          string => test-chain-id
	GenesisModifiers []genesis.Modifier => []genesis.Modifier{}

Copy link
Member Author

Choose a reason for hiding this comment

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

ok thanks Rootul, then let's do it

Copy link
Member Author

Choose a reason for hiding this comment

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

done

return &Testnet{
seed: seed,
seed: opts.Seed,
nodes: make([]*Node, 0),
genesis: genesis.NewDefaultGenesis().WithChainID(chainID).WithModifiers(genesisModifiers...),
keygen: newKeyGenerator(seed),
grafana: grafana,
knuu: kn,
genesis: genesis.NewDefaultGenesis().WithChainID(opts.ChainID).WithModifiers(opts.GenesisModifiers...),
keygen: newKeyGenerator(opts.Seed),
grafana: opts.Grafana,
knuu: opts.Knuu,
}, nil
}

func (t *Testnet) Knuu() *knuu.Knuu {
return t.knuu
}

Comment on lines -62 to -65
Copy link
Collaborator

Choose a reason for hiding this comment

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

[question] is this getter no longer helpful?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really as the user passes the knuu object, so they can use it on their end the way they need.

func (t *Testnet) NewPreloader() (*preloader.Preloader, error) {
mojtaba-esk marked this conversation as resolved.
Show resolved Hide resolved
if t.knuu == nil {
return nil, errors.New("knuu is not initialized")
Expand Down
Loading