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: Remove the dataset indexer images and the indexer.feature + utilize sandbox for algod/indexer/postgres containers #220

Merged
merged 76 commits into from
Aug 22, 2022

Conversation

tzaffi
Copy link
Contributor

@tzaffi tzaffi commented Aug 3, 2022

Summary

After removing the 4 non-live indexer instances and realizing that the live one wasn't operational, it was easier to swap out all the docker containers with the much more stable sandbox.

TODO

  • Get consensus on the best way to move forward with this PR. In particular, I see a few approaches:
    • Most Aggressive - Stop testing all v1 endpoints. This will require significant investigation into what the v1 endpoint-dependencies are inside of SDK steps implementations and implicitly their features.
    • Medium - Continue with the current PR and leave the V1 Endpoint Problem to a future PR. The current approach requires the step removal/modifications described below.
    • Least Disruptive - instead of removing features and steps -as is done in this PR- re-tag everything so that obsolete steps are marked @obsolete. This requires much more detailed tagging of steps.
    • Something else?
  • Have all SDK's removed their step dependency on indexer v1? If so, this PR can be merged. Details:
    • [@rekey_v1 and @send tags] - then("the transaction should go through") DO NOT assert "type" in context.acl.transaction_by_id(context.txn.get_txid()) because this relies on indexer v1
    • [@applications.verified tag] - step("I wait for the transaction to be confirmed.") DO NOT assert "type" in context.acl.transaction_by_id(context.app_txid)

Related PR's

Files introduced/modified/dropped:

  • Introducing .env to help configure the sandbox. This replaces .up-env
  • config.harness is a sandbox template used for standing it up
  • docker-compose.yml and docker/***are removed as they are subsumed by sandbox's docker build artifacts
  • features/integration/ - removed integration test features:
    • evaldelta.feature - this hasn't been running since February 2022
    • indexer.feature and features/resources/v23x*/*** fixtures - this was a static test last modified March 2021 and which didn't interact with algod
  • network_config/ - removing all configurations for bootstrapping the algod node. As Sandbox is now in charge of bringing up the network, all configurations are "owned" by it. If in the future, a network that doesn't exist in Sandbox is required by SDK testing, it is recommended that this network be introduced to Sandbox via a pull request.
  • scripts/ - restructured the test scripts:
    • restart.sh - this is removed as sandbox already has ./sandbox up which serves the same purpose in a robust fashion
    • down.sh - delegates to ./sandbox down and ./sandbox clean
    • up.sh - serves a similar purpose as before, delegating logic based on channel / source configuration and also populating the config.harness template to be utilized in /sandbox up config.harness
  • utils/ - removing all files except for utils/implements_all_tests.sh. The removed files include:
    • create_indexer_integration_goldens.sh test case generator that depended on the non-live indexers being removed
    • utils/create_transactions.sh an old test case generating script.
    • *teal and *tok files which weren't used in any tests. These have been subsumed by the programs in features/resources/programs/
  • utils/unused_steps_analysis.ipynb - Script which uses a gherkin parser to aggregate all feature steps, and then uses regular expressions and fuzzy string logic to help discover unused steps in each SDK. This helped discover an average of 12 obsolete steps in each of the SDK's. If we were to start running nightly jobs in this repo, and with a bit more TLC, this could be automated to alert of unused steps.

Steps Removal Guide

The following steps have been orphaned due to the removal of indexer.feature and evaldelta.feature:

  1. when('I use {indexer} to lookup account "{account}" at round {round}')
  2. when('I use {indexer} to search for an account with {assetid}, {limit}, {currencygt}, {currencylt}, "{auth_addr:MaybeString}", {application_id}, "{include_all:MaybeBool}" and token "{token:MaybeString}"')
  3. when('I use {indexer} to search for an account with {assetid}, {limit}, {currencygt}, {currencylt}, "{auth_addr:MaybeString}", {application_id} and token "{token:MaybeString}"')
  4. when('I use {indexer} to search for an account with {assetid}, {limit}, {currencygt}, {currencylt} and token "{token:MaybeString}"')
  5. when("I get the next page using {indexer} to search for an account with {assetid}, {limit}, {currencygt} and {currencylt}")
  6. when('I use {indexer} to search for applications with {limit}, {application_id}, "{include_all:MaybeBool}" and token "{token:MaybeString}"')
  7. when('I use {indexer} to search for applications with {limit}, {application_id}, and token "{token:MaybeString}"')
  8. when('I use {indexer} to lookup application with {application_id} and "{include_all:MaybeBool}"')
  9. when("I use {indexer} to lookup application with {application_id}")
  10. when("I get the next page using {indexer} to lookup asset balances for {assetid} with {currencygt}, {currencylt}, {limit}")
  11. when("I use {indexer} to search for all {assetid} asset transactions")
  12. when('I use {indexer} to search for all "{accountid}" transactions')
  13. when("I use {indexer} to check the services health")
  14. when("I use {indexer} to lookup block {number}")
  15. when('I use {indexer} to lookup asset balances for {assetid} with {currencygt}, {currencylt}, {limit} and token "{token}"')
  16. when("I use {indexer} to lookup asset {assetid}")
  17. when("I get the next page using {indexer} to search for transactions with {limit} and {maxround}")
  18. when('I use {indexer} to search for transactions with {limit}, "{noteprefix:MaybeString}", "{txtype:MaybeString}", "{sigtype:MaybeString}", "{txid:MaybeString}", {block}, {minround}, {maxround}, {assetid}, "{beforetime:MaybeString}", "{aftertime:MaybeString}", {currencygt}, {currencylt}, "{address:MaybeString}", "{addressrole:MaybeString}", "{excludecloseto:MaybeString}" and token "{token:MaybeString}"')
  19. when('I use {indexer} to search for transactions with {limit}, "{noteprefix:MaybeString}", "{txtype:MaybeString}", "{sigtype:MaybeString}", "{txid:MaybeString}", {block}, {minround}, {maxround}, {assetid}, "{beforetime:MaybeString}", "{aftertime:MaybeString}", {currencygt}, {currencylt}, "{address:MaybeString}", "{addressrole:MaybeString}", "{excludecloseto:MaybeString}", {application_id} and token "{token:MaybeString}"')
  20. when('I use {indexer} to search for assets with {limit}, {assetidin}, "{creator:MaybeString}", "{name:MaybeString}", "{unit:MaybeString}", and token "{token:MaybeString}"')
  21. when('indexer client {index} at "{address}" port {port} with token "{token}"')
  22. then("I get transactions by address only") - needs indexer v1 [@algod tag]
  23. then("I can get the transaction by ID") - needs indexer v1 [@algod tag]
  24. when("I get recent transactions, limited by {cnt} transactions") - [@algod tag]
  25. then("I get transactions by address and date") - [@algod tag]

Additional indexer.feature steps which were previously overlooked, but need removal.

These are uniquely discoverable (unless noted otherwise) by the following search strings:
26. I receive status code
27. transactions, has the previous block hash
28. has a frozen status of
29. with a total amount of
30. μalgos
31. The asset found has:
32. with the asset, the first
33. , the first has
34. The first account is online and has
35. transactions in the response, the first is
36. Every transaction has tx-type
37. Every transaction has sig-type
38. Every transaction has round >=
39. Every transaction has round <=
40. Every transaction has round
41. Every transaction works with asset-id
42. Every transaction is older than
43. Every transaction is newer than
44. Every transaction moves between
45. assets in the response, the first is
46. the parsed response should equal - CAREFUL!!! there are unit as well as integration test versions. Make sure to only remove the indexer integration test version
47. the unconfirmed pending transaction by ID (actually an evaldelta feature)
48. the confirmed pending transaction by ID

Steps being modified (for example in the Python SDK)

  • [@rekey_v1 and @send tags] - then("the transaction should go through")
    • DO NOT assert "type" in context.acl.transaction_by_id(context.txn.get_txid()) because this relies on indexer v1
  • [@applications.verified tag] - step("I wait for the transaction to be confirmed.")
    • DO NOT assert "type" in context.acl.transaction_by_id(context.app_txid)

@tzaffi tzaffi marked this pull request as draft August 3, 2022 23:20
docker-compose.yml Outdated Show resolved Hide resolved
scripts/up.sh Outdated Show resolved Hide resolved
config.harness Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
@tzaffi tzaffi changed the title WIP: remove the dataset indexer images and the indexer.feature WIP: remove the dataset indexer images and the indexer.feature + utilize sandbox for algod/indexer/postgres containers Aug 8, 2022
README.md Outdated Show resolved Hide resolved
Co-authored-by: Michael Diamant <[email protected]>
scripts/up.sh 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.

@tzaffi Thank you for your investigative work + multiple cycles to bring a net improvement in testing experience + build times!

@tzaffi
Copy link
Contributor Author

tzaffi commented Aug 16, 2022

@tzaffi Thank you for your investigative work + multiple cycles to bring a net improvement in testing experience + build times!

Thanks for approving. I will not merge this PR unitll all the "TODO" checkboxes in the PR description have been checked - indicating that all the dependencies are in place for merging.

Copy link
Contributor

@winder winder left a comment

Choose a reason for hiding this comment

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

Great! Thanks for digging into this and figuring out how many deprecated/duplicate tests we have.

Do you have any idea how many steps this removes?

I found this plugin for Java, maybe the other languages have something similar: https://blog.jdriven.com/2019/01/cucumber-jvm-plugin/

@tzaffi
Copy link
Contributor Author

tzaffi commented Aug 17, 2022

Great! Thanks for digging into this and figuring out how many deprecated/duplicate tests we have.

Do you have any idea how many steps this removes?

I found this plugin for Java, maybe the other languages have something similar: https://blog.jdriven.com/2019/01/cucumber-jvm-plugin/

Currently, there are 48 steps in the "Steps Removal Guide". @algochoi - 26 thru 48 are the new ones which I identified after I approved the Javascript PR. Most helpful was actually the go-sdk which organized the indexer feature steps all in one place. Next up, I'll be doing Java and will try to use the tool you linked (now I wish I had started with Java first). I'll update the "Steps Removal Guide" with any more deletions that I discover.

@@ -4,28 +4,28 @@ Feature: Compile

@compile
Scenario Outline: Compile programs
When I compile a teal program <program>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Making variable templating more compliant with SDK expectations.

@tzaffi tzaffi requested a review from michaeldiamant August 22, 2022 13:47
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 Thanks for taking in several rounds of feedback - Looking forward to merging!

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.

4 participants