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

test: add integration tests for sidechain transactions #571

Merged
merged 22 commits into from
Nov 28, 2023

Conversation

mvadari
Copy link
Collaborator

@mvadari mvadari commented May 3, 2023

High Level Overview of Change

Title says it all.

Context of Change

Integration tests for #417. These tests are in a separate PR to make it easier to review and because tests won't pass in CI yet (since the amendment isn't a part of the Docker container yet), and will be merged into the feature branch prior to merging into main. Any bugfixes that were caught by these tests were moved to #417.

Type of Change

  • Tests (You added tests for code that already exists, or your new feature included in this PR)

Test Plan

Passes locally when running a version of rippled with the amendment enabled.

@mvadari mvadari changed the base branch from master to sidechain-2.5 May 3, 2023 20:29
@mvadari mvadari mentioned this pull request May 3, 2023
2 tasks
@mvadari mvadari force-pushed the sidechain-2.5-integ branch 2 times, most recently from 4800772 to a42327c Compare June 8, 2023 20:27
@mvadari mvadari force-pushed the sidechain-2.5-integ branch 2 times, most recently from 7c23abf to 6e251c8 Compare June 26, 2023 19:50
@mvadari mvadari force-pushed the sidechain-2.5-integ branch 3 times, most recently from 42cc378 to 77534d0 Compare July 10, 2023 17:12
@mvadari mvadari marked this pull request as ready for review September 14, 2023 21:50
@mvadari mvadari changed the title [DO NOT MERGE] test: add integration tests for sidechain transactions test: add integration tests for sidechain transactions Sep 14, 2023
@mvadari mvadari mentioned this pull request Sep 21, 2023
2 tasks
Base automatically changed from sidechain-2.5 to master September 27, 2023 15:35
@mvadari mvadari changed the base branch from master to rippleci October 24, 2023 18:15
Base automatically changed from rippleci to master October 24, 2023 18:54
@mvadari mvadari force-pushed the sidechain-2.5-integ branch 2 times, most recently from 4912251 to 5f0d788 Compare November 13, 2023 19:59
for node in created_nodes
if node["LedgerEntryType"] == "XChainOwnedClaimID"
]
assert len(claim_ids_ledger_entries) == 1, len(claim_ids_ledger_entries)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this using a different assert than the rest of the testing framework?

Comment on lines 34 to 49
claim_id_hash = (
claim_id_response.result.get("tx_json") or claim_id_response.result
)["hash"]
claim_id_tx_response = await client.request(Tx(transaction=claim_id_hash))

nodes = claim_id_tx_response.result["meta"]["AffectedNodes"]
created_nodes = [
node["CreatedNode"] for node in nodes if "CreatedNode" in node.keys()
]
claim_ids_ledger_entries = [
node
for node in created_nodes
if node["LedgerEntryType"] == "XChainOwnedClaimID"
]
assert len(claim_ids_ledger_entries) == 1, len(claim_ids_ledger_entries)
xchain_claim_id = claim_ids_ledger_entries[0]["NewFields"]["XChainClaimID"]
Copy link
Contributor

@JST5000 JST5000 Nov 21, 2023

Choose a reason for hiding this comment

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

These variables are a bit hard to read because they all start with a long prefix. I'd recommend simplifying the names by removing that prefix (claim_id_...), and potentially adding a comment near the top for // Claim ID Data so that the variable's purposes can be more easily sight-read.

I've been having this problem a lot when reviewing sidechain PRs (since there's a lot of XChain... variables as well - so I'd recommend trying to apply this change more broadly.

Suggested change
claim_id_hash = (
claim_id_response.result.get("tx_json") or claim_id_response.result
)["hash"]
claim_id_tx_response = await client.request(Tx(transaction=claim_id_hash))
nodes = claim_id_tx_response.result["meta"]["AffectedNodes"]
created_nodes = [
node["CreatedNode"] for node in nodes if "CreatedNode" in node.keys()
]
claim_ids_ledger_entries = [
node
for node in created_nodes
if node["LedgerEntryType"] == "XChainOwnedClaimID"
]
assert len(claim_ids_ledger_entries) == 1, len(claim_ids_ledger_entries)
xchain_claim_id = claim_ids_ledger_entries[0]["NewFields"]["XChainClaimID"]
hash = (
claim_id_response.result.get("tx_json") or claim_id_response.result
)["hash"]
tx_response = await client.request(Tx(transaction=hash))
nodes = tx_response.result["meta"]["AffectedNodes"]
created_nodes = [
node["CreatedNode"] for node in nodes if "CreatedNode" in node.keys()
]
ledger_entries = [
node
for node in created_nodes
if node["LedgerEntryType"] == "XChainOwnedClaimID"
]
assert len(ledger_entries) == 1, len(ledger_entries)
xchain_claim_id = ledger_entries[0]["NewFields"]["XChainClaimID"]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd rather keep the variables more descriptive.

Copy link
Contributor

@JST5000 JST5000 left a comment

Choose a reason for hiding this comment

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

LGTM - Just a variable name style comment to take or leave :)

Copy link
Collaborator

@khancode khancode left a comment

Choose a reason for hiding this comment

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

LGTM

@mvadari mvadari merged commit d1e92b7 into main Nov 28, 2023
18 checks passed
@mvadari mvadari deleted the sidechain-2.5-integ branch November 28, 2023 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants