Skip to content

Testing: Modify cucumber steps to use dev mode network#360

Merged
algochoi merged 38 commits into
developfrom
dev-mode-testing
Aug 1, 2022
Merged

Testing: Modify cucumber steps to use dev mode network#360
algochoi merged 38 commits into
developfrom
dev-mode-testing

Conversation

@algochoi
Copy link
Copy Markdown
Contributor

@algochoi algochoi commented Jul 8, 2022

This PR introduces some instrumentation to support dev mode testing (add zero payment transaction to advance blocks in certain scenarios) and delete v1 algod cucumber tests.

v1 API Changes

There are two changes to the v1 steps:

  • Added a transient dev mode account to safely send some transactions after a potential rekey transaction.
  • Removed some algod calls in the transaction should go through step, especially ones that test recent block rounds (harder to test in dev mode as blocks need to be manually advanced and may not be recent).

Experimental results

Initial testing looks like just deleting v1 algod steps will have a ~7 min speedup (21min -> 14min)

Dev mode, disabling wait_for_transaction v1 step (~10 min):

@algochoi algochoi changed the title Testing: Modify cucumber steps to use dev mode network and delete v1 algod steps Testing: Modify cucumber steps to use dev mode network Jul 15, 2022
Comment thread run_integration.sh Outdated
# Reset test harness
rm -rf test-harness
git clone --single-branch --branch master https://github.com/algorand/algorand-sdk-testing.git test-harness
git clone --single-branch --branch devmodenet https://github.com/algorand/algorand-sdk-testing.git test-harness
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Related SDK Testing PR: algorand/algorand-sdk-testing#206

Comment thread Makefile Outdated
behave --tags=$(UNITS) tests -f progress2

INTEGRATIONS = "@abi or @algod or @applications or @applications.verified or @assets or @auction or @c2c or @compile or @dryrun or @dryrun.testing or @indexer or @indexer.231 or @indexer.applications or @kmd or @rekey or @send.keyregtxn or @send or @compile.sourcemap"
INTEGRATIONS = "@abi or @algod or @applications or @applications.verified or @assets or @auction or @c2c or @compile or @dryrun or @dryrun.testing or @indexer or @indexer.231 or @indexer.applications or @kmd or @send.keyregtxn or @send or @compile.sourcemap"
Copy link
Copy Markdown
Contributor Author

@algochoi algochoi Jul 20, 2022

Choose a reason for hiding this comment

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

The existing v1 rekeying tests alters the default wallet key which causes problems in sending transactions after rekeying in dev mode - I decided to just remove it here since v1 APIs are deprecated anyway, but a more complete solution would be to re-write them so that they alter a transient private key instead.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you know if this problem is universal across SDK's?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Do you know if this problem is universal across SDK's?

I haven't looked into all of them, but I think it would be. I also think the order of execution matters - it poses the least amount of problems if @rekey is executed last.

I did push a workaround hack for the v1 functions in my latest commit - I created a transient dev account with some funds to send transactions before a potential rekey.

@algochoi algochoi marked this pull request as ready for review July 20, 2022 15:45
Copy link
Copy Markdown
Contributor

@tzaffi tzaffi left a comment

Choose a reason for hiding this comment

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

I haven't tested locally yet, but I plan to. I have some questions mostly regarding transaction amounts and the number of rounds needed to be waited for in dev mode.

Also some minor suggestions.

Comment thread tests/steps/application_v2_steps.py Outdated
def wait_for_app_txn_confirm(context):
sp = context.app_acl.suggested_params()
last_round = sp.first
send_zero_transactions(context, 3)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Curious on how the number 3 was arrived at.

Copy link
Copy Markdown
Contributor Author

@algochoi algochoi Jul 21, 2022

Choose a reason for hiding this comment

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

In the line below, it checks the status after block last_round + 2, so I sent 3 transactions (advances 3 blocks in dev mode) for good measure!

I'll add a small comment here to explain this

Comment thread tests/steps/other_v2_steps.py Outdated
Comment thread tests/steps/other_v2_steps.py Outdated
Comment thread tests/steps/other_v2_steps.py Outdated
Comment thread tests/steps/steps.py
Comment thread tests/steps/steps.py Outdated
Comment thread tests/steps/other_v2_steps.py Outdated
Comment thread tests/steps/steps.py Outdated
Comment thread tests/steps/other_v2_steps.py Outdated
context.accounts[0],
sp,
constants.ZERO_ADDRESS,
random.randint(100000, 900000),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@algochoi Optionally, I propose configuring the upper bound proportional to the funding amount. Intent is to avoid generating a value that's too large if/when the funding amount changes.

Here's the Java SDK analog: algorand/java-algorand-sdk@3390a6d.

Feel welcomed to resolve as is.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point - Added proportional funding amounts in latest commit 3f11057

Comment thread tests/steps/steps.py
Copy link
Copy Markdown
Contributor

@michaeldiamant michaeldiamant left a comment

Choose a reason for hiding this comment

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

@algochoi Great work here - looks ready to me. ☕

Approval presumes the following prior to merge:

  • Revert algorand-sdk-testing branch to master.
  • Remove NETWORK_TEMPLATE override. Assumes upstream defaults to DevModeNetwork.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants