-
Notifications
You must be signed in to change notification settings - Fork 98
Deploy flags e2e #2755
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
Deploy flags e2e #2755
Conversation
"happyPath": [ | ||
{ | ||
"name": "local_deploy", | ||
"flags": {} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can set globalFlags in each testCases as well, which will override the globalFlags value
var _ = ginkgo.Describe("[Blockchain Deploy Flags]", ginkgo.Ordered, func() { | ||
_ = ginkgo.BeforeEach(func() { | ||
// Create test subnet config | ||
commands.CreateSubnetEvmConfigSOV(subnetName, utils.SubnetEvmGenesisPath, ewoqEVMAddress) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use ewoq here because we use ewoq as validator manager owner when we deploy in local network
} | ||
|
||
// TestCommandWithJSONConfig tests a CLI command with flag inputs from a JSON file | ||
func TestCommandWithJSONConfig(command string, configPath string, testCase *TestCase) (string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
love the idea - any plan to move it to some test common directories to enable it reusable by all other commands?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Yes i will move it to external directories
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generalized the function
Example of how this PR reduces code while increasing code coverage: Currently, when we are testing key transfer:
We have to write a custom function to call Compare that with using Using JSON ensures that every possible flag is tested, compared to doing this right now, where we can't verify that we have tested every scenario:
|
@@ -85,7 +85,7 @@ func CreateSubnetEvmConfigWithVersionNonSOV(subnetName string, genesisPath strin | |||
gomega.Expect(exists).Should(gomega.BeTrue()) | |||
} | |||
|
|||
func CreateSubnetEvmConfigWithVersionSOV(subnetName string, genesisPath string, version string) { | |||
func CreateSubnetEvmConfigGenesisPathWithVersionSOV(subnetName string, genesisPath string, version string, validatorManagerOwner string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need a generic function that just take the command name, the non flag args, and a map for flags, and
stopping doing this
@@ -236,9 +236,9 @@ func CreateCustomVMConfigSOV(subnetName string, genesisPath string, vmPath strin | |||
genesisPath, | |||
"--proof-of-authority", | |||
"--validator-manager-owner", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we also need to stop using flag constants here, and take those from cmd/flags
@@ -35,6 +35,7 @@ jobs: | |||
"\\[Node create\\]", | |||
"\\[Node devnet\\]", | |||
"\\[Docker\\]", | |||
"\\[Blockchain Deploy Flags\\]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure we want to start modifying CI at this stage
@@ -0,0 +1,22 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this cmd tree. probably this can have a default name, like test_table.json
type CommandGroup string | ||
|
||
const ( | ||
BlockchainCmd CommandGroup = "blockchain" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather add this constant on cmd/blockchaincmd/ and import that here
tests/e2e/commandse2e/command.go
Outdated
// TestCase represents a single test case configuration | ||
type TestCase struct { | ||
Name string `json:"name"` | ||
Flags map[string]string `json:"flags"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You also need non flag args here
} | ||
|
||
// Append any additional arguments | ||
if len(args) > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should belong to the test case
ginkgo.By(fmt.Sprintf("Running test case: %s", testCase.Name)) | ||
_, err = commandse2e.TestCommandWithJSONConfig( | ||
commandse2e.BlockchainCmd, | ||
"deploy", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be a constant on cmd/blockchaincmd/
// Cleanup test subnet config | ||
commands.DeleteSubnetConfig(subnetName) | ||
}) | ||
blockchainCmdArgs := []string{subnetName} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should belong to the json test case IMO
var _ = ginkgo.Describe("[Blockchain Deploy Flags]", ginkgo.Ordered, func() { | ||
_ = ginkgo.BeforeEach(func() { | ||
// Create test subnet config | ||
commands.CreateSubnetEvmConfigSOV(subnetName, ewoqEVMAddress, commands.PoA) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this should be added to the json spec (how to setup required env)
Our current E2E for tests/e2e/testcases is pretty extensive for local network across all commands. However, it does not test flags input extensively for happy path and error path.
For example, our E2E tests for blockchain deploy in https://github.com/ava-labs/avalanche-cli/blob/main/tests/e2e/testcases/subnet/sov/ currently tests that we are able to deploy to local network using PoA / PoS / Initialize Validator Manager. However, it does not test the multiple ways that we can achieve each objectives (for example we can use deploy an L1 with by setting num-local-machine=2 instead of using default value of 1).
This PR proposes using json files as input that is fed into a general function (TestCommandWithJSONConfig), which can be reused for all blockchain subcommands (e.g. blockchain addValidator, etc) simply by feeding it with different JSON file. Reusing same core testing logic enables us to achieve extensive happy path and error path coverage without huge additional testing code.