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

Enhancement: Use Sandbox for Testing #360

Merged
merged 17 commits into from
Aug 22, 2022
Merged

Enhancement: Use Sandbox for Testing #360

merged 17 commits into from
Aug 22, 2022

Conversation

tzaffi
Copy link
Contributor

@tzaffi tzaffi commented Aug 16, 2022

+= Sandbox -= Indexer Snapshots

Undertaking the work of removing obsolete indexer steps and testing using the Sandbox.

Defining Issue

algorand/algorand-sdk-testing#221

TODO

  • Has trim-indexer branch of algorand-sdk-testing been merged?
    • Repoint the dependency here to algorand-sdk-testing/master
    • Now we can merge

Summary of Changes

  • .test-env - new test-only environment file
  • Makefile - more fine grained control over the test process in line with other SDK's
  • test-harness.sh - replaces test/docker/run_docker.sh and is in line with the new approach of other SDK's
  • test/:
    • applications_integration_test.go - step removal
    • applications_unit_test.go - removal of unused helper
    • docker/ remove run_docker.sh and sdk.py
  • helpers.go- removing functions no longer used after step deletion
  • indexer_integration_test.go - removed as only used by indexer.feature
  • steps_test.go - step removal
  • transactions_test.go - step removal

Step Removals

Please refer to the step removal guide in the companion SDK Testing PR.

Additional Steps being removed following Unused Steps Analysis script

See this google sheet

  • s.Step('^we make a Search For Transactions call with account "([^"]*)" NotePrefix "([^"]*)" TxType "([^"]*)" SigType "([^"]*)" txid "([^"]*)" round (\d+) minRound (\d+) maxRound (\d+) limit (\d+) beforeTime (\d+) afterTime (\d+) currencyGreaterThan (\d+) currencyLessThan (\d+) assetIndex (\d+) addressRole "([^"]*)" ExcluseCloseTo "([^"]*)"$' ...)
  • s.Step('^the parsed Pending Transactions Information response should have sender "([^"]*)"$' ...)
  • s.Step('^we make a Pending Transaction Information against txid "([^"]*)" with max (\d+)$' ...)
  • s.Step('^we make a Pending Transactions By Address call against account "([^"]*)" and max (\d+)$' ...)
  • s.Step('^we make a Get Block call against block number (\d+)$' ...)
  • s.Step('^we make a SearchForAssets call with limit (\d+) creator "([^"]*)" name "([^"]*)" unit "([^"]*)" index (\d+) and afterAsset (\d+)$' ...)
  • s.Step("I create the payment transaction" ...)
  • s.Step('I read a transaction "([^"]*)" from file "([^"]*)"' ...)
  • s.Step("I write the transaction to file" ...)
  • s.Step("the transaction should still be the same" ...)
  • s.Step("I do my part" ...)
  • s.Step('key registration transaction parameters (\d+) (\d+) (\d+) "([^"]*)" "([^"]*)" "([^"]*)" (\d+) (\d+) (\d+) "([^"]*)" "' ...)
  • s.Step("I create the key registration transaction", ...)

Timing Stats

@tzaffi tzaffi marked this pull request as draft August 16, 2022 22:48
@tzaffi tzaffi marked this pull request as ready for review August 17, 2022 04:47
@tzaffi tzaffi requested a review from algochoi August 17, 2022 05:23
Copy link
Contributor

@algochoi algochoi left a comment

Choose a reason for hiding this comment

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

Nice, LGTM - one comment about error handling

test/steps_test.go Outdated Show resolved Hide resolved
test-harness.sh Outdated Show resolved Hide resolved
.test-env Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@c2c
@compile
@compile.sourcemap
@dryrun
Copy link
Contributor Author

Choose a reason for hiding this comment

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

deleted tags:

@indexer
@indexer.231
@indexer.applications

@@ -0,0 +1,26 @@
@unit.abijson
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should be the same tags

Copy link
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.

@tzaffi Nice work here - Thanks for the extra cycles to minimize error when dealing with Cucumber tag configuration.

Aside: I like the spirit of REMOVE_LOCAL_FEATURES - it supports faster local iteration. Though I'm slightly concerned it will lead to difficult to debug states.

  • Overall, I think it's worth trialing.
  • In a future iteration, if we think it's worth pursuing, a possible alternative I'm imagining is to supply configuration analogous to sandbox's TYPE="CHANNEL" / TYPE="SOURCE" to conditionally choose the algorand-sdk-testing source. The intent being to find an algorand-sdk-testing installation on the local filesystem.

@tzaffi
Copy link
Contributor Author

tzaffi commented Aug 18, 2022

@tzaffi Nice work here - Thanks for the extra cycles to minimize error when dealing with Cucumber tag configuration.

Aside: I like the spirit of REMOVE_LOCAL_FEATURES - it supports faster local iteration. Though I'm slightly concerned it will lead to difficult to debug states.

  • Overall, I think it's worth trialing.
  • In a future iteration, if we think it's worth pursuing, a possible alternative I'm imagining is to supply configuration analogous to sandbox's TYPE="CHANNEL" / TYPE="SOURCE" to conditionally choose the algorand-sdk-testing source. The intent being to find an algorand-sdk-testing installation on the local filesystem.

I introduced REMOVE_LOCAL_FEATURES when I realized that the previous behavior in the SDK's did not wipe out new features, so I didn't want to spring any surprises on anyone who was expecting the maintenance of this behavior. But I can see the advantages of just wiping clean each time and not needing a switch. I don't feel strongly about this.

@@ -1,6 +1,9 @@
SRCPATH := $(shell pwd)
TEST_SOURCES := $(shell cd $(SRCPATH) && go list ./...)
TEST_SOURCES_NO_CUCUMBER := $(shell cd $(SRCPATH) && go list ./... | grep -v test)
UNIT_TAGS := "$(shell awk '{print $2}' test/unit.tags | paste -s -d, -)"
INTEGRATIONS_TAGS := "$(shell awk '{print $2}' test/integration.tags | paste -s -d, -)"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cucumber tags now reside in:

  • test/unit.tags
  • test/integration.tags

Makefile Outdated
cd test && go test -timeout 0s --godog.strict=true --godog.format=pretty --godog.tags=$(INTEGRATIONS_TAGS) --test.v .

display-all-go-steps:
find test -name "*.go" | xargs grep "github.com/cucumber/godog" 2>/dev/null | cut -d: -f1 | sort | uniq | xargs grep -Eo "Step[(].[^\`]+" | awk '{sub(/:Step\(./,":")} 1' | sed -E 's/", [a-zA-Z0-9]+\)//g' #| grep "with path"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Command that generates text used by Unused Steps Analysis script

.test-env Outdated Show resolved Hide resolved
Copy link
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.

Approval assumes #360 (comment) is addressed prior to merge.

Co-authored-by: Michael Diamant <[email protected]>
Makefile Outdated Show resolved Hide resolved
@tzaffi tzaffi merged commit 0bc0ce7 into develop Aug 22, 2022
@tzaffi tzaffi deleted the sandbox-sdk-testing branch August 22, 2022 16:25
almog-t pushed a commit to almog-t/go-algorand-sdk that referenced this pull request Aug 29, 2022
ahangsu added a commit that referenced this pull request Sep 2, 2022
* Github-Actions: Adding pr title and label checks (#336)

* Dev Tools: Source map decoder (#335)

* Updated CHANGELOG.md

* AVM:  Consolidate TEAL and AVM versions (#345)

* Testing: Use Dev mode network for cucumber tests (#349)

* AVM: Use avm-abi repo (#352)

* Updated CHANGELOG.md

* Enhancement: Use Sandbox for Testing (#360)

Co-authored-by: Michael Diamant <[email protected]>

* Bug-Fix: passthru verbosity (#371)

* Enhancement: Deprecating use of langspec (#366)

Co-authored-by: Michael Diamant <[email protected]>

* StateProof: State proof support (#374)

* Regenerate client.

* Update txn and block objects, cucumber tests passing.

* Enable cucumber tests.

* Update types/stateproof.go

* Add transactions-root-256

* Remove unused code.

* Remove unused file

* Add txn commitments blockheader fields

* Rename signature, add allocbound definitions

* Add txn commitment to header

* PR comments

* Remove unused file

Co-authored-by: Will Winder <[email protected]>

* State Proofs: Use generic type for StateProof txn field. (#378)

* Use generic type for StateProof txn field.

* Add missing type.

* Remove GenericDigest.

* fix type assertion on source map version (#370)

* let the json package figure out how to decode the elements of the incoming map (#380)

* StateProof: State Proof Verification additions (#377)

* Regenerate client.

* Remove unused file

* moved files from stateproof verification repo

* now using updated go-stateproof-verification

* fixed go mod

* moved consts near their usage

* added working state proof verifier test

* added unit test and resources

* now using go 1.15 supported function

* changes to avoid dependency of stateproof verification on SDK

* added needed types to stateproof.go for message hashing

* reverted packages, now using up to date go-stateproof-verification

* CR: renamed verification functions

* hashing sp message is now in crypto

* removed hashing of light block header

* CR: now allocating space upfront in hashing state proof message

* attempting to use embedded files

* CR: using GenericDigest in block.go

* CR: Renamed package to match directory

* Removed go 1.15 from CI

* improved embed usage

* CR: removed stateproof verification logic

* tidied go mod file

Co-authored-by: Will Winder <[email protected]>
Co-authored-by: Eric Warehime <[email protected]>

* State Proofs: added compute leaf function for light block header to sdk (#382)

* added compute leaf function for light block header to sdk

* renamed blockheader256

* changed passed type to pointer

* reverted back to copying

* renamed light block header hash func (#383)

* Enable stpf cucumber unit tests (#386)

Co-authored-by: Jack <[email protected]>
Co-authored-by: Ben Guidarelli <[email protected]>
Co-authored-by: Jack Smith <[email protected]>
Co-authored-by: Michael Diamant <[email protected]>
Co-authored-by: algochoi <[email protected]>
Co-authored-by: Jason Paulos <[email protected]>
Co-authored-by: Barbara Poon <[email protected]>
Co-authored-by: Zeph Grunschlag <[email protected]>
Co-authored-by: Eric Warehime <[email protected]>
Co-authored-by: Will Winder <[email protected]>
Co-authored-by: Almog Tal <[email protected]>
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