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

Restructure tests #15

Merged
merged 4 commits into from
Aug 13, 2020
Merged

Conversation

jasonpaulos
Copy link
Member

@jasonpaulos jasonpaulos commented Aug 3, 2020

Move AST tests from tests/test_ops.py into individual testing files in the pyteal directory. The first commit only moves tests, while the second commit restructures the individual tests and adds checking for generated intermediate code.

@jasonpaulos jasonpaulos mentioned this pull request Aug 5, 2020
@derbear
Copy link

derbear commented Aug 13, 2020

A proposal for testing intermediate code (that might be good to add to a different PR):

It may be possible to automatically generate differential tests that catch if some detail of low-level code generation changes from one version of PyTEAL to the next. There could be a test generator which exhaustively runs through the opcodes (and maybe small combinations of them) and outputs the expected values of these compiled opcodes; if these expected values are checked into the git repo, then we can have unit tests that check that we match these values. If we change how code generation works from one version to the next, then these tests will catch that.

I think we do something similar in the TEAL assembler here:
https://github.com/algorand/go-algorand/blob/master/data/transactions/logic/assembler_test.go#L217-L247

pyteal/ast/bytes.py Outdated Show resolved Hide resolved
pyteal/types.py Outdated Show resolved Hide resolved
@@ -51,6 +52,7 @@ def test_atomic_swap():
&&
||
&&"""
reset_label_count()
Copy link

Choose a reason for hiding this comment

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

Out of curiosity, why are these required here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Those are required in general to reset the global label counter in util.py so that the labels match the expected code string. Although it looks like this example does not actually use labels, so that call can be removed from here if you think it's confusing.

Copy link

@derbear derbear Aug 13, 2020

Choose a reason for hiding this comment

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

Ah, okay--that makes sense.

I don't think it's necessary to remove them; rather, it seems somewhat redundant that you'd just place it before every test for the tests to be correct.

To a larger point, it's a little unfortunate that state for these labels is global and not already implicitly reset per invocation of compileTeal, which is what intuitively makes sense to me. Perhaps there's a way to move this global counter to somewhere local to compileTeal.

@jasonpaulos
Copy link
Member Author

@derbear that testing proposal sounds like a good idea. I've created an issue for it: #17

Copy link

@derbear derbear left a comment

Choose a reason for hiding this comment

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

Looks good to me. Might want to make a small note in the merge that the way byte is compiled is now slightly different.

@jasonpaulos jasonpaulos merged commit a67fd6e into algorand:master Aug 13, 2020
@jasonpaulos jasonpaulos deleted the test-restructure branch August 13, 2020 20:17
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.

None yet

2 participants