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

CI: Expose configuration to ignore parent E2E expect test runner invocations #4422

Merged
merged 2 commits into from
Aug 17, 2022

Conversation

michaeldiamant
Copy link
Contributor

Summary

Extends scripts/buildtools/check_tests.py to customize tests to ignore via --ignored-tests. The motivation is to acquiesce check_tests.py warnings about E2E expect tests run multiple times.

In another branch, I saw warnings in https://app.circleci.com/pipelines/github/algorand/go-algorand/8787/workflows/a566ab26-42ad-48de-940d-08d504ce1c15/jobs/158801?invite=true#step-103-7. Reproduced below for convenience:

=========== RAN MULTIPLE TIMES ===================
The above 4 tests ran multiple times:
github.com/algorand/go-algorand/test/e2e-go/cli/algod/expect TestAlgodWithExpect -- ran 10 times. (Can probably be fixed by adding "partitiontest.PartitionTest()")
github.com/algorand/go-algorand/test/e2e-go/cli/algoh/expect TestAlgohWithExpect -- ran 10 times. (Can probably be fixed by adding "partitiontest.PartitionTest()")
github.com/algorand/go-algorand/test/e2e-go/cli/goal/expect TestGoalWithExpect -- ran 10 times. (Can probably be fixed by adding "partitiontest.PartitionTest()")
github.com/algorand/go-algorand/test/e2e-go/cli/tealdbg/expect TestTealdbgWithExpect -- ran 10 times. (Can probably be fixed by adding "partitiontest.PartitionTest()")
The above 4 tests ran multiple times:
==================================================

Upon inspection, I realized these tests are parent E2E test runners for expect tests. They're invoked multiple times by-design and the advice to add partitioning does not apply.

As a band aid, the PR exposes a way to ignore these tests. See the PR's builds for proof that the warnings disappear.

  • Treat the PR optionally. I didn't see a way to prevent the tests from reporting via go test flags. I can envision managing the ignore list as worse than the status quo.
  • I realize the verification step happens for all test types. The configuration relies on test name uniqueness.
  • For the PR's scope, I'm gauging if the band aid is deemed a net positive. If it isn't, I'd prefer to close and move on.

Test Plan

Run CI.

@codecov
Copy link

codecov bot commented Aug 17, 2022

Codecov Report

Merging #4422 (baf8f59) into master (f156f69) will decrease coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4422      +/-   ##
==========================================
- Coverage   55.18%   55.13%   -0.06%     
==========================================
  Files         397      397              
  Lines       50073    50073              
==========================================
- Hits        27634    27606      -28     
- Misses      20150    20170      +20     
- Partials     2289     2297       +8     
Impacted Files Coverage Δ
ledger/roundlru.go 90.56% <0.00%> (-5.67%) ⬇️
util/db/dbutil.go 44.24% <0.00%> (-4.25%) ⬇️
ledger/blockqueue.go 85.63% <0.00%> (-2.88%) ⬇️
crypto/merkletrie/trie.go 66.42% <0.00%> (-2.19%) ⬇️
crypto/merkletrie/node.go 91.62% <0.00%> (-1.87%) ⬇️
cmd/tealdbg/debugger.go 72.69% <0.00%> (-0.81%) ⬇️
ledger/acctupdates.go 69.92% <0.00%> (-0.61%) ⬇️
network/wsPeer.go 67.67% <0.00%> (-0.55%) ⬇️
ledger/acctonline.go 78.47% <0.00%> (-0.53%) ⬇️
network/wsNetwork.go 64.89% <0.00%> (+0.19%) ⬆️
... and 1 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@jannotti jannotti left a comment

Choose a reason for hiding this comment

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

Good find.

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.

2 participants