CI: add test partitioning based on partition test verifier output #3859
Conversation
|
|
| // TestEvalAppStateCountsWithTxnGroupBlackbox ensures txns in a group can't violate app state schema limits | ||
| // the test ensures that | ||
| // commitToParent -> applyChild copies child's cow state usage counts into parent | ||
| // and the usage counts correctly propagated from parent cow to child cow and back | ||
| func TestEvalAppStateCountsWithTxnGroup(t *testing.T) { | ||
| func TestEvalAppStateCountsWithTxnGroupBlackbox(t *testing.T) { | ||
| partitiontest.PartitionTest(t) | ||
|
|
||
| _, _, err := testEvalAppGroup(t, basics.StateSchema{NumByteSlice: 1}) | ||
| require.Error(t, err) | ||
| require.Contains(t, err.Error(), "store bytes count 2 exceeds schema bytes count 1") | ||
| } | ||
|
|
||
| // TestEvalAppAllocStateWithTxnGroup ensures roundCowState.deltas and applyStorageDelta | ||
| // TestEvalAppAllocStateWithTxnGroupBlackbox ensures roundCowState.deltas and applyStorageDelta | ||
| // produce correct results when a txn group has storage allocate and storage update actions | ||
| func TestEvalAppAllocStateWithTxnGroup(t *testing.T) { | ||
| func TestEvalAppAllocStateWithTxnGroupBlackbox(t *testing.T) { |
There was a problem hiding this comment.
I certainly see nothing wrong with changing these names, but I'm curious if it mattered for some reason.
There was a problem hiding this comment.
the skipped / multiply-run test counting checker in scripts/buildtools/check_tests.py uses the package + test name from the JUnit file to ensure all tests ran at least once (i.e. no tests were skipped by all shards). It got confused about there being two identically named tests in this package (there is also TestEvalAppStateCountsWithTxnGroup and TestEvalAppAllocStateWithTxnGroup in eval_test.go), so it was reporting it as not having been correctly sharded because the package naming for the blackbox test allows the same test name to be re-used.
There was a problem hiding this comment.
Do we have any easy way to catch that in the future, or conversely, can the checker take file name into account?
And wait - you can have two TestX() functions in the same package?!
There was a problem hiding this comment.
the JUnit file only reports package and test names, so no ... but I was surprised too. It's because eval_test.go uses "package internal" and eval_blackbox_test.go uses "package internal_test" so the names don't conflict with each other — they're in two different package namespaces
There was a problem hiding this comment.
Also, the checker's main job is to ensure no test was skipped by all shards, so it is still correct even if this happens in the future.
Summary
We have a test partitioning system that uses calls to partitiontest.PartitionTest(t) to shard tests. There is also a verification job to check for accidentally skipped or unnecessarily multiply-run tests. This cleans up some of the "RAN MULTIPLE TIMES" warnings of unnecessarily multiply-run tests from output like https://app.circleci.com/pipelines/github/algorand/go-algorand/5910/workflows/d5620b33-8dd4-4211-a1b1-1780a792126a/jobs/99253 and https://app.circleci.com/pipelines/github/algorand/go-algorand/5907/workflows/ee653582-9329-43f2-87fc-51dc047d2e3d/jobs/99171
Test Plan
Existing tests should pass, and there should be fewer "RAN MULTIPLE TIMES" warnings in the verification job output. There should be no "UNINTENTIONALLY SKIPPED" errors in the output (this would trigger a CI error).