Refactor TestReturnTypes to generate a test case for each opcode

[michaeldiamant]

Summary

Refactors TestReturnTypes in data/transactions/logic/evalStateful_test.go to generate a test case for each opcode + run mode. Previously, a test case is generated only for run mode (2 tests).

By generating unique tests per opcode + run mode, a failing opcode won't prevent testing of other opcodes.

Here's local output cataloging the change:

• Before the PR:

~/dev/go-algorand/data/transactions/logic avm-error-handling *1 ?2 ────────────────────────────────── 11:17:17 AM
❯ gotestsum -- . --run "TestReturnTypes"
✓  data/transactions/logic (cached)

DONE 3 tests in 0.778s

• With PR (note increase in test count):

~/dev/go-algorand/data/transactions/logic avm-error-handling !3 ?2 ────────────────────────────────── 11:17:37 AM
❯ gotestsum -- . --run "TestReturnTypes"
✓  data/transactions/logic (cached)

DONE 252 tests in 0.766s

• Illustrates how deliberately failing a test still allows other opcodes to be tested:

 ~/dev/go-algorand/data/transactions/logic avm-error-handling +1 !1 ?2 ───────────────────────── ✘ 255 11:19:55 AM
❯ git --no-pager diff evalStateful_test.go
diff --git a/data/transactions/logic/evalStateful_test.go b/data/transactions/logic/evalStateful_test.go
index 85a081ef..6044f3d4 100644
--- a/data/transactions/logic/evalStateful_test.go
+++ b/data/transactions/logic/evalStateful_test.go
@@ -2366,7 +2366,7 @@ func TestReturnTypes(t *testing.T) {
 		"itxnas":      ": itxn_begin; int pay; itxn_field TypeEnum; itxn_submit; int 0; itxnas Accounts",
 		"gitxn":       "itxn_begin; int pay; itxn_field TypeEnum; itxn_submit; gitxn 0 Sender",
 		"gitxna":      "itxn_begin; int pay; itxn_field TypeEnum; itxn_submit; gitxna 0 Accounts 0",
-		"gitxnas":     ": itxn_begin; int pay; itxn_field TypeEnum; itxn_submit; int 0; gitxnas 0 Accounts",
+		"gitxnas":     ": itxn_begin; int pay; itxn_field TypeEnum; itxn_submit; int 1; gitxnas 0 Accounts",

 		"base64_decode": `: byte "YWJjMTIzIT8kKiYoKSctPUB+"; base64_decode StdEncoding`,
 		"json_ref":      `: byte "{\"k\": 7}"; byte "k"; json_ref JSONUint64`,

~/dev/go-algorand/data/transactions/logic avm-error-handling +1 !1 ?2 ─────────────────────────────── 11:20:06 AM
❯ gotestsum -- . --run "TestReturnTypes"
✖  data/transactions/logic (300ms)

=== Failed
=== FAIL: data/transactions/logic TestReturnTypes/mode=Application,opcode=gitxnas (0.00s)
    evalStateful_test.go:2469:
        	Error Trace:	evalStateful_test.go:2469
        	Error:      	Received unexpected error:
        	            	invalid Accounts index 1
        	Test:       	TestReturnTypes/mode=Application,opcode=gitxnas
        	Messages:   	gitxnas: invalid Accounts index 1
        	            	  1 intcblock 1 => <empty stack>
        	            	  4 itxn_begin => <empty stack>
        	            	  5 intc_0 // 1 => (1 0x1)
        	            	  6 itxn_field TypeEnum => <empty stack>
        	            	  8 itxn_submit => <empty stack>
        	            	  9 intc_0 // 1 => (1 0x1)
        	            	 10 gitxnas 0 Accounts =>
        	            	 10 invalid Accounts index 1
    --- FAIL: TestReturnTypes/mode=Application,opcode=gitxnas (0.00s)

=== FAIL: data/transactions/logic TestReturnTypes (0.02s)

DONE 252 tests, 2 failures in 2.435s

Test Plan

Documented in summary.

[jannotti]

I think I like this change, since the independent failures are noted more clearly, but actually the previous test does allow two opcodes to fail because of the use of assert, rather than require. Go has the (unintuitive?) semantics that an aasert lets the test keep running, but reports the error and treats the test as failing once its done. So you can collect multiple problems. require fails immediately. So I had recently made this test use assert for exactly this reason. So, if you have a mistake in gitxnas AND itxnas, you get:

go test -run TestReturnTypes\$ . 
--- FAIL: TestReturnTypes (0.01s)
    --- FAIL: TestReturnTypes/m=Application (0.00s)
        evalStateful_test.go:2469: 
            	Error Trace:	evalStateful_test.go:2469
            	Error:      	Received unexpected error:
            	            	invalid Accounts index 1
            	Test:       	TestReturnTypes/m=Application
            	Messages:   	gitxnas: invalid Accounts index 1
            	            	  1 intcblock 1 => <empty stack>
            	            	  4 itxn_begin => <empty stack>
            	            	  5 intc_0 // 1 => (1 0x1) 
            	            	  6 itxn_field TypeEnum => <empty stack>
            	            	  8 itxn_submit => <empty stack>
            	            	  9 intc_0 // 1 => (1 0x1) 
            	            	 10 gitxnas 0 Accounts => 
            	            	 10 invalid Accounts index 1
        evalStateful_test.go:2469: 
            	Error Trace:	evalStateful_test.go:2469
            	Error:      	Received unexpected error:
            	            	invalid Accounts index 1
            	Test:       	TestReturnTypes/m=Application
            	Messages:   	itxnas: invalid Accounts index 1
            	            	  1 intcblock 1 => <empty stack>
            	            	  4 itxn_begin => <empty stack>
            	            	  5 intc_0 // 1 => (1 0x1) 
            	            	  6 itxn_field TypeEnum => <empty stack>
            	            	  8 itxn_submit => <empty stack>
            	            	  9 intc_0 // 1 => (1 0x1) 
            	            	 10 itxnas Accounts => 
            	            	 10 invalid Accounts index 1

Note the two (different) "Messages" lines.

If this test didn't already have the t.Run business, I'd probably not want the added complexity, but since it already did, and this just moves it, I like it.

[michaeldiamant]

@jannotti Thanks for the explanation. ☕

I did not understand assert behavior.

I had tried a few cases locally before opening the PR. And now I'm thinking I misinterpreted whatever I saw. In any event, I'll keep the point in mind for the future.