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

e2e: Ensure interchain workflow coverage for the P-Chain #1882

Merged
merged 12 commits into from
Sep 21, 2023
Merged

e2e: Ensure interchain workflow coverage for the P-Chain #1882

merged 12 commits into from
Sep 21, 2023

Conversation

marun
Copy link
Contributor

@marun marun commented Aug 20, 2023 •
edited
Loading

Why this should be merged

Fulfills one of the requirements for #1547 (migration of Kurtosis tests)

How this works

How this was tested

CI

TODO

@marun marun added testing This primarily focuses on testing e2e-migrate-kurtosis labels Aug 20, 2023
@marun marun mentioned this pull request Aug 20, 2023
Closed
16 tasks
hexfusion
hexfusion approved these changes Aug 20, 2023
View reviewed changes
tests/e2e/faultinjection/duplicate_node_id.go Outdated Show resolved Hide resolved
@marun marun force-pushed the e2e-p-chain-workflow branch 2 times, most recently from 064d475 to 19987b2 Compare August 22, 2023 00:09
@marun marun linked an issue Aug 23, 2023 that may be closed by this pull request
Migrate Kurtosis Tests #1547
Closed
16 tasks
@marun marun self-assigned this Aug 23, 2023
@marun marun force-pushed the e2e-p-chain-workflow branch 2 times, most recently from 1a6e8de to 2996199 Compare August 27, 2023 19:18
@marun marun marked this pull request as ready for review August 27, 2023 19:18
@marun marun requested review from abi87 and gyuho as code owners August 27, 2023 19:18
@marun marun requested a review from danlaine August 27, 2023 20:07
@marun marun marked this pull request as draft August 30, 2023 16:37
@marun marun force-pushed the e2e-p-chain-workflow branch 6 times, most recently from 4475f2d to 34feccc Compare September 1, 2023 18:33
@marun marun mentioned this pull request Sep 1, 2023
Merged
@marun marun force-pushed the e2e-p-chain-workflow branch 2 times, most recently from d785771 to d30a8ac Compare September 7, 2023 01:13
@marun marun marked this pull request as ready for review September 7, 2023 01:14
StephenButtolph
StephenButtolph reviewed Sep 20, 2023
View reviewed changes
tests/e2e/p/interchain_workflow.go Outdated Show resolved Hide resolved
tests/e2e/p/interchain_workflow.go Outdated Show resolved Hide resolved
@marun
Copy link
Contributor Author

marun commented Sep 21, 2023

Rebased

abi87
abi87 reviewed Sep 21, 2023
View reviewed changes
tests/e2e/e2e.go Outdated Show resolved Hide resolved
abi87
abi87 reviewed Sep 21, 2023
View reviewed changes
tests/e2e/p/interchain_workflow.go
nodeID, nodePOP, err := infoClient.GetNodeID(e2e.DefaultContext())
require.NoError(err)

ginkgo.By("adding the new node as a validator", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

q: why do we need to promote the node as validator? Can't we let it be a non-validator issuing a import/export Tx and let other validators in the network do the job of accepting the transactions?

Copy link
Contributor Author

@marun marun Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The requirement in #1547 is for A P-Chain workflow test where we add a node to the validator set, add a delegator for that node, and do import/export transactions to the X-Chain and C-Chain.

abi87
abi87 reviewed Sep 21, 2023
View reviewed changes
tests/e2e/p/interchain_workflow.go
require.NoError(err)
})

ginkgo.By("adding a delegator to the new node", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

q: why do we need to make nodeID as both validator and delegator?
Also IIUC, what we are doing here is delegating some more nodeID's funds to nodeID. Why is this needed?

Copy link
Contributor Author

@marun marun Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Again, the requirement in #1547 is for A P-Chain workflow test where we add a node to the validator set, add a delegator for that node, and do import/export transactions to the X-Chain and C-Chain.

abi87
abi87 previously requested changes Sep 21, 2023
View reviewed changes
Copy link
Contributor

@abi87 abi87 left a comment

Choose a reason for hiding this comment

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

a couple of questions about test structure, which may be simpler. Other than that LGTM

@StephenButtolph StephenButtolph dismissed abi87’s stale review September 21, 2023 15:47

comments were addressed

@StephenButtolph StephenButtolph added this to the v1.10.11 milestone Sep 21, 2023
@StephenButtolph StephenButtolph merged commit fd36124 into dev Sep 21, 2023
16 checks passed
@StephenButtolph StephenButtolph deleted the e2e-p-chain-workflow branch September 21, 2023 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hexfusion hexfusion hexfusion approved these changes

@danlaine danlaine danlaine approved these changes

@StephenButtolph StephenButtolph StephenButtolph approved these changes

@abi87 abi87 abi87 left review comments

@gyuho gyuho Awaiting requested review from gyuho

Assignees

@marun marun

Labels
testing This primarily focuses on testing
Projects
No open projects
Ski Patrol
Archived in project
Milestone
v1.10.11
Development

Successfully merging this pull request may close these issues.

Migrate Kurtosis Tests
5 participants
@marun @hexfusion @danlaine @abi87 @StephenButtolph