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.

Sign up for GitHub

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

Jump to bottom

Burn Transaction Test + Refactors in Basic #627

Merged
merged 47 commits into from
Dec 13, 2022
Merged

Burn Transaction Test + Refactors in Basic #627

merged 47 commits into from
Dec 13, 2022

Conversation

cxloe
Copy link
Contributor

@cxloe cxloe commented Dec 8, 2022 •
edited
Loading

Soundtrack to this PR

Motivation

Integration test coverage for Burn Transactions. Working towards standard, reusable, relatively parsable testing helper functions.

In this PR

  • Short test for burn transactions. Follows roughly the same convention as previous tests, but with less duplicated code.
  • Renamed and refactored utility functions. TestUtils class ( to be moved out of basic later. )
  • Namely, functions that wait for all accounts to sync, and a function that waits two blocks. Those may be redundant but it's what i ended up with..
  • Some clearer named variables.
  • accounts.py is a script i ended up writing to print the ID, balance and entropy of all my imported FS accounts.
  • More typing for fullservice.py
  • pytest compatibility for basic.py and burn_transaction_test.py

Test Plan

poetry run python burn_transaction_test.py

To Do

Make sure async def wait_for_account_sync(): works right.

Future Work

  • Get things organized! Pull out TestUtils into it's own file, and sort out the folder structure.
  • ( in a sarcastic tone of voice ) DATACLASSES SO I NEVER HAVE TO INVOKE .get EVER AGAIN IN MY LIFE I S2G IF I SEE ANOTHER NoneType ERR-

cxloe added 30 commits December 5, 2022 19:04
@cxloe
another sleep
2de27a9
@cxloe
utility to check balance and export all accounts
292dcc0
@cxloe
remove dataobjects
b8dce7a
@cxloe
initial stab at importing things seperately, test skeleton
620e878
@cxloe
start on wait for sync, handle not being main
008d29f
@cxloe
Start refactoring into TestUtils class
4f7d3cb
@cxloe
rename functions, cleanup
236ddf0
@cxloe
conciseness
5171989
@cxloe
black
01a3029
@cxloe
comment
8446b51
@cxloe
setup imports
df84fe3
@cxloe
import accounts func
8c025f6
@cxloe
fold into single function
1b89549
@cxloe
build the burn transaction
c903150
@cxloe
black
fd6f0e7
@cxloe
whoops
6085710
@cxloe
checking balances
1eeb705
@cxloe
clean up accounts helper function
bb78fbb
@cxloe
clean accounts, testing calls to check for sync
a319dcf
@cxloe
start on waiting for the next block
056b8bf
@cxloe
first pass at waiting for the next block
b9fd64f
@cxloe
cleaner way to wait for the next block
b9a4535
@cxloe
wait for account sync function
ee5e0f5
@cxloe
restore entrypoint
8d5f36d
@cxloe
wait two blocks
f2408b2
@cxloe
rename fix
cc50452
@cxloe
formatting
0f8f19e
@cxloe
we always need to open the config!!
ee2fddf
@cxloe
fix renamed func calls
ee6c2f8
@cxloe
we can just wait for all the accounts to sync
639a369
@cxloe cxloe requested a review from a team December 8, 2022 02:10
@cxloe cxloe marked this pull request as ready for review December 8, 2022 02:11
pouneh
pouneh approved these changes Dec 8, 2022
View reviewed changes
.internal-ci/test/fs-integration/basic.py Outdated Show resolved Hide resolved
@cxloe cxloe requested a review from a team December 8, 2022 18:17
@cxloe cxloe force-pushed the zd/burn_transaction_test branch 2 times, most recently from 914dfcd to 84273bb Compare December 8, 2022 18:54
pouneh
pouneh previously requested changes Dec 8, 2022
View reviewed changes
Copy link
Contributor

@pouneh pouneh 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 it makes most sense for burn_transaction_test.py to live with the other integration tests and not under python-library

@cxloe
Copy link
Contributor Author

cxloe commented Dec 9, 2022 •
edited
Loading

I think it makes most sense for burn_transaction_test.py to live with the other integration tests and not under python-library

Folder structure is out of scope for this, will revisit in a later PR. I think python-library/integration-tests makes sense. CI will be able to invoke whatever tests it's configured to run by importing from the same place it needs to import fullservice.py

@cxloe cxloe requested review from pouneh and a team December 9, 2022 00:37
cxloe added 2 commits December 12, 2022 17:01
@cxloe
pytest for main test (#629)
16eeddd 
* seeing what needs to change..

* revert globals

* seems to work!

* instanciate testutils
@cxloe
shorter and sweeter wait for sync
3e8dcc4
@cxloe cxloe marked this pull request as draft December 12, 2022 22:05
@cxloe cxloe marked this pull request as ready for review December 12, 2022 22:27
@cxloe cxloe dismissed pouneh’s stale review December 12, 2022 22:42

no change was suggested

queenbdubs
queenbdubs approved these changes Dec 13, 2022
View reviewed changes
Copy link

@queenbdubs queenbdubs left a comment

Choose a reason for hiding this comment

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

Just had questions as I'm learning about all this

@cxloe cxloe merged commit c2e872f into develop Dec 13, 2022
@cxloe cxloe deleted the zd/burn_transaction_test branch December 13, 2022 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@queenbdubs queenbdubs queenbdubs approved these changes

@pouneh pouneh Awaiting requested review from pouneh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cxloe @codecov-commenter @pouneh @queenbdubs