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

citogo - a Go test wrapper that solves a bunch of CI problems #22322

Merged
merged 2 commits into from Feb 3, 2020

Conversation

@maxtaco
Copy link
Contributor

maxtaco commented Feb 1, 2020

  • run one test in a process, so they don't leak threads and memory
  • output failed logs to files, so the logs don't become insane
  • in CI, output logs to S3, for the above reason
  • allow a certain number of flakes (3)
  • after 3 failures, don't keep running, just bail early.
@maxtaco maxtaco force-pushed the maxtaco/ci-magic branch 2 times, most recently from a563f10 to 22c4dec Feb 2, 2020
@maxtaco maxtaco marked this pull request as ready for review Feb 2, 2020
@maxtaco maxtaco changed the title wip citogo - a Go test wrapper that solves a bunch of CI problems Feb 2, 2020
@maxtaco maxtaco requested review from jzila and mmaxim Feb 2, 2020
@jzila

This comment has been minimized.

Copy link
Contributor

jzila commented Feb 2, 2020

I dont have time to look at this today, but please dont merge until I've taken a look tomorrow.

@maxtaco

This comment has been minimized.

Copy link
Contributor Author

maxtaco commented Feb 2, 2020

No rush! I think it's failing CI becuase "recursively fail to remove directory" is failing and it's the last thing in the run.

@maxtaco maxtaco force-pushed the maxtaco/ci-magic branch 3 times, most recently from 5d5e533 to c1cf59b Feb 3, 2020
- run one test in a process, so they don't leak threads and memory
- output failed logs to files, so the logs don't become insane
- in CI, output logs to S3, for the above reason
- allow a certain number of flakes (3)
- after 3 failures, don't keep running, just bail early.
@maxtaco maxtaco force-pushed the maxtaco/ci-magic branch from c1cf59b to c976a94 Feb 3, 2020
@patrickxb

This comment has been minimized.

Copy link
Contributor

patrickxb commented Feb 3, 2020

i think some stats -> stathat would be helpful to see how CI is doing.

@maxtaco

This comment has been minimized.

Copy link
Contributor Author

maxtaco commented Feb 3, 2020

this framework makes that totally feasible, since now we can notate individual test runs. We can add that in a subsequent PR

@mmaxim
mmaxim approved these changes Feb 3, 2020
@maxtaco

This comment has been minimized.

Copy link
Contributor Author

maxtaco commented Feb 3, 2020

OK, this thing is ready to go and pretty well-tested at this point. Changes to Jenkinsfile are minimal

@jzila
jzila approved these changes Feb 3, 2020
Copy link
Contributor

jzila left a comment

LGTM pending one question.

sh "./${testBinary} -test.timeout ${testSpec.timeout}"
def t = getOverallTimeout(testSpec)
timeout(activity: true, time: t.time, unit: t.unit) {
sh "citogo --flakes 3 --fails 3 --build ${env.BRANCH_NAME}_${env.BUILD_ID} --prefix ${testSpec.dirPath} --s3bucket ci-fail-logs --build-url ${env.BUILD_URL} --get-log-cmd /keybase/team/keybase/bin/ciget --no-compile --test-binary ./${testBinary}"

This comment has been minimized.

Copy link
@jzila

jzila Feb 3, 2020

Contributor

What is this --get-log-cmd and how does it have access to the keybase team?

This comment has been minimized.

Copy link
@maxtaco

maxtaco Feb 3, 2020

Author Contributor

it's for displaying how to get the log, but it's not actually getting the log

@maxtaco maxtaco merged commit a7fb0d8 into master Feb 3, 2020
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/jenkins/pr-head This commit cannot be built
Details
ci/circleci Your tests passed on CircleCI!
Details
@maxtaco maxtaco deleted the maxtaco/ci-magic branch Feb 3, 2020
}

func (r *runner) listTests() error {
cmd := exec.Command(r.testerName(), "-test.list", ".")

This comment has been minimized.

Copy link
@jzila

jzila Feb 3, 2020

Contributor

TIL -test.list 🤯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.