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 SDK Testing and remove Indexer v1 steps #623

Merged
merged 12 commits into from
Aug 22, 2022

Conversation

algochoi
Copy link
Contributor

@algochoi algochoi commented Aug 16, 2022

Python SDK PR here: algorand/py-algorand-sdk#367

.test-env Outdated Show resolved Hide resolved
@algochoi algochoi changed the title Enhancement: Use sandbox for SDK Testing Enhancement: Use sandbox for SDK Testing and remove Indexer v1 steps Aug 16, 2022
Copy link
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.

Looking good so far. Still expecting a few more mods (if JS is similar to Python).

Step removal:

  • then("I get transactions by address only")
  • then("I can get the transaction by ID")
  • when("I get recent transactions, limited by {cnt} transactions")
  • then("I get transactions by address and date")

Step modification:

  • step("I wait for the transaction to be confirmed.")

@algochoi algochoi requested a review from tzaffi August 16, 2022 21:11
@algochoi
Copy link
Contributor Author

@tzaffi I think I made all the necessary deletions now. For step("I wait for the transaction to be confirmed."), the JS SDK does not support v1 in the first place, so I didn't make any modifications there.

Copy link
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 think the tests aren't running, unless I'm missing something?

That would explain how the algod step that I was surprised was removed didn't break the test.

tests/cucumber/steps/steps.js Outdated Show resolved Hide resolved
@tzaffi
Copy link
Contributor

tzaffi commented Aug 16, 2022

I think the tests aren't running, unless I'm missing something?

That would explain how the algod step that I was surprised was removed didn't break the test.

better link: https://app.circleci.com/pipelines/github/algorand/js-algorand-sdk/448/workflows/639ed9bb-2aac-4d14-a6cf-242f53c3d3cf/jobs/2543?invite=true#step-110-1860

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

Approving with the provision that this PR should wait on the SDK-testing trim-indexer branch being merged and then be pointed to master.

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

optional suggestion - I'm defaulting to no longer wiping out the existing features, and introducing a verbose mode

.test-env Show resolved Hide resolved
test-harness.sh Outdated Show resolved Hide resolved
@tzaffi tzaffi self-requested a review August 17, 2022 20:42
Copy link
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.

Re-approving after confirming the newly deleted steps against the expected list.

I'll keep you posted if I discover any new possible deletions.

Makefile Outdated Show resolved Hide resolved
@michaeldiamant michaeldiamant marked this pull request as ready for review August 18, 2022 20:24
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.

Approving with the understanding that .test-env branch reverted to master prior to merge.

…hose steps (#627)

* command to generate input for Unused Steps Analysis Script + Remove those steps

* remvoe the steps

* In response to CR remark. Update Makefile
.test-env Outdated Show resolved Hide resolved
Co-authored-by: Zeph Grunschlag <[email protected]>
@algochoi algochoi merged commit cc370dc into develop Aug 22, 2022
@algochoi algochoi deleted the sandbox-sdk-testing branch August 22, 2022 17:47
ahangsu added a commit that referenced this pull request Sep 3, 2022
* bump version and add to changelog

* update README.md for new version

* prettier on CHANGELOG

* Update README.md

Co-authored-by: John Lee <[email protected]>

* Update README.md

Co-authored-by: John Lee <[email protected]>

* Enhancement: Use sandbox for SDK Testing and remove Indexer v1 steps (#623)

* Add files to enable sandbox sdk testing

* Add some todo comments and formatting

* Update changes to script and env

* Delete indexer v1 test steps

* More changes to cucumber tests

* Change features directory in JS SDK

* Remove more indexer integration tests

* Update test env

* Update test-harness.sh

Co-authored-by: Zeph Grunschlag <[email protected]>

* Break out cucumber tags into their own files (#625)

* Command to generate input for Unused Steps Analysis Script + Remove those steps (#627)

* command to generate input for Unused Steps Analysis Script + Remove those steps

* remvoe the steps

* In response to CR remark. Update Makefile

* Update .test-env

Co-authored-by: Zeph Grunschlag <[email protected]>

Co-authored-by: Zeph Grunschlag <[email protected]>

* Bugfix: Pass verbosity to the harness and sandbox (#630)

* Ignore algorand-sdk-testing test-harness dir (#634)

* Enhancement: Deprecating use of langspec (#632)

* enhancement: Initial stateproofs support (#629)

* Initial stateproofs support

* Some more unused steps (#637)

* dummy push

* revert change of dummy push, ci runs now

Co-authored-by: Barbara Poon <[email protected]>
Co-authored-by: algobarb <[email protected]>
Co-authored-by: John Lee <[email protected]>
Co-authored-by: algochoi <[email protected]>
Co-authored-by: Zeph Grunschlag <[email protected]>
Co-authored-by: Michael Diamant <[email protected]>
Co-authored-by: Eric Warehime <[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