-
Notifications
You must be signed in to change notification settings - Fork 69
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
feat: add in simulate function for ATC and EmptyTransactionSigner #728
base: develop
Are you sure you want to change the base?
feat: add in simulate function for ATC and EmptyTransactionSigner #728
Conversation
a5c393f
to
10043d4
Compare
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 for this contribution. I've left comments about improvements I'd like to see, and guidance for how to add support for our existing simulation integration tests.
src/main/java/com/algorand/algosdk/transaction/AtomicTransactionComposer.java
Outdated
Show resolved
Hide resolved
src/main/java/com/algorand/algosdk/transaction/AtomicTransactionComposer.java
Outdated
Show resolved
Hide resolved
src/main/java/com/algorand/algosdk/transaction/AtomicTransactionComposer.java
Outdated
Show resolved
Hide resolved
src/main/java/com/algorand/algosdk/transaction/AtomicTransactionComposer.java
Outdated
Show resolved
Hide resolved
src/main/java/com/algorand/algosdk/transaction/AtomicTransactionComposer.java
Outdated
Show resolved
Hide resolved
src/main/java/com/algorand/algosdk/transaction/AtomicTransactionComposer.java
Show resolved
Hide resolved
src/main/java/com/algorand/algosdk/transaction/AtomicTransactionComposer.java
Outdated
Show resolved
Hide resolved
src/main/java/com/algorand/algosdk/transaction/SignedTransaction.java
Outdated
Show resolved
Hide resolved
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.
While this example may have been useful for your testing, I'm not sure how benefitial it is to check it into the repo.
In any case, we already have a set of test cases designed around SDK simulation abilities. They belong in our cucumber testing repo and their definitions are here: https://github.com/algorand/algorand-sdk-testing/blob/master/features/integration/simulate.feature. In order to approve this PR, I'd like to see these tests passing here.
You can enable them by adding @simulate
to src/test/integration.tags
. Then locally you can do the following to run the tests:
# Start the cucumber testing harness using docker composer
make harness
# Run the cucumber integration tests.
# Note, for more rapid iteration, you can temporarily make "@simulate" the only tag in src/test/integration.tags
# That may be beneficial anyway, since some of the other integration tests have problems running multiple times on the same network
make integration
# When done, this shuts down the testing harness
make harness-down
The first time you run make integration
the testing tool will complain about missing cucumber step definitions and provide you an outline of what it thinks the definitions should be. You will need to add new functions to implement these step definitions. Existing step definitions can be found in src/test/java/com/algorand/algosdk/integration/AtomicTxnComposer.java
and similar files, and for referene the Go SDK steps, which already have support for @simulate
, are here: https://github.com/algorand/go-algorand-sdk/blob/dde38d8d139e1587b401a3301f1c2fe74dd55c8b/test/applications_integration_test.go
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.
@jasonpaulos - Sorry on taking awhile to update this PR, it took me some time to configure my local environment for cucumber tests and getting the necessary free time to work on this. I had a question though (after I understood your feedback)...does the Go Algorand SDK
include all the @simulate
steps in simulate.feature
? I was using it as a reference and noticed some missing
I removed the reti
example test I wrote from this PR since simulate.feature
test coverage would probably be sufficient
Please also let me know if I'm not on the right path on with the simulate.feature
tests (i committed the first batch of integration tests into this PR now for review)
Summary
AtomicTransactionComposer.java
(mirror Go/JS versions)EmptyTransactionSigner
classTesting Plan
simulate.feature
cucumber tests