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.

Sign up for GitHub

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

Jump to bottom

switch go tests to json output #80822

Merged
merged 3 commits into from Aug 1, 2019
Merged

switch go tests to json output #80822

merged 3 commits into from Aug 1, 2019

Conversation

liggitt
Copy link
Member

@liggitt liggitt commented Jul 31, 2019 •
edited

What type of PR is this?
/kind bug
/kind failing-test
/kind flake

What this PR does / why we need it:
Switches go tests to json output and switches junit generator to github.com/gotestyourself/gotestsum to avoid flakes caused by spurious output parsing

Which issue(s) this PR fixes:
Fixes #80258

Tested:

  • ensure successful runs parse correctly
  • introduce a test failure and ensure it gets identified and fails the test jobs

Does this PR introduce a user-facing change?:

NONE

/sig testing
/cc @BenTheElder

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. kind/flake Categorizes issue or PR as related to a flaky test. sig/testing Categorizes an issue or PR as relevant to SIG Testing. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/dependency Issues or PRs related to dependency changes labels Jul 31, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 31, 2019
@liggitt liggitt changed the title WIP: switch to json output WIP: switch go tests to json output Jul 31, 2019
@BenTheElder
Copy link
Member

looks like a straightforward win! 👀 on CI
/assign

@BenTheElder
Copy link
Member

there seems to be some bug verifying the LICENSE?

something seems to think gotest.tools/ needs one, we have one for gotest.tools/gotestsum

@BenTheElder
Copy link
Member

I think you need to also vendor https://github.com/gotestyourself/gotest.tools/ ?

@liggitt liggitt force-pushed the test-json-output branch 2 times, most recently from c32543e to 0423af4 Compare July 31, 2019 20:20
@liggitt
Copy link
Member Author

liggitt commented Jul 31, 2019

junit output for successful integration test run looks identical (in terms of test names and number of tests)

old:

new:

@BenTheElder
Copy link
Member

the log file is bigger for the new one, but also the same order of magnitude so /shrug
the rest of the files appear to be approximately the same size.

@liggitt
Copy link
Member Author

liggitt commented Jul 31, 2019

Makes sense that wrapping metadata around each line would add some overhead

@BenTheElder
Copy link
Member

/priority important-soon
/retest

@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jul 31, 2019
@liggitt
Copy link
Member Author

liggitt commented Jul 31, 2019

The integration failure was intentional:
https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/80822/pull-kubernetes-integration/1156682058571976704/

@BenTheElder
Copy link
Member

BenTheElder commented Jul 31, 2019 via email

@liggitt
Copy link
Member Author

liggitt commented Jul 31, 2019

dropped the intentionally failing commit, this is ready for review

@liggitt liggitt changed the title WIP: switch go tests to json output switch go tests to json output Jul 31, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 31, 2019
BenTheElder
BenTheElder approved these changes Aug 1, 2019
View reviewed changes
Copy link
Member

@BenTheElder BenTheElder left a comment

Choose a reason for hiding this comment

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

/lgtm
/hold
one suggestion / question

hack/make-rules/test.sh

if ! command -v gotestsum >/dev/null 2>&1; then
kube::log::error "gotestsum not found; please install with " \
"go get -u gotest.tools/gotestsum"
Copy link
Member

Choose a reason for hiding this comment

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

should we be telling people to install from vendor instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

this was mostly a copy/paste of the existing message... given our scripts do the install from vendor, I don't really care either way

Copy link
Member

Choose a reason for hiding this comment

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

i figured as much, imho we should suggest doing what we would do, otherwise they're going to run it with a different version? /shrug

Copy link
Member

Choose a reason for hiding this comment

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

let's revisit later actually ... @cblecker penny for your thoughts ...

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Aug 1, 2019
@liggitt
Copy link
Member Author

liggitt commented Aug 1, 2019

/retest

@BenTheElder
Copy link
Member

/hold cancel
thanks for doing this!

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 1, 2019
@liggitt
Copy link
Member Author

liggitt commented Aug 1, 2019

/retest

there's the TestVolumeProvision flake we know and love

@k8s-ci-robot k8s-ci-robot merged commit 3771044 into kubernetes:master Aug 1, 2019
@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Aug 1, 2019
@liggitt liggitt deleted the test-json-output branch August 1, 2019 01:46
@cblecker
Copy link
Member

cblecker commented Aug 1, 2019

retroactive lgtm. thanks for this @liggitt

cc: @kubernetes/sig-testing FYI

@Huang-Wei
Copy link
Member

I noticed with this PR, the command make test-integration WHAT=./test/integration/XYZ ... will generate tons of (JSON based) logs. While in the before, the log is very neat (at least upon test success).

@liggitt
Copy link
Member Author

liggitt commented Aug 1, 2019

We could only do json output if a junit report is requested

@liggitt liggitt mentioned this pull request Aug 1, 2019
Merged
@liggitt
Copy link
Member Author

liggitt commented Aug 1, 2019

follow up in https://github.com/kubernetes/kubernetes/pull/80863/files

@Huang-Wei
Copy link
Member

Thanks @liggitt. #80863 sounds good.

@ixdy ixdy mentioned this pull request Mar 3, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenTheElder BenTheElder BenTheElder approved these changes

Assignees

@BenTheElder BenTheElder

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/dependency Issues or PRs related to dependency changes area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. kind/flake Categorizes issue or PR as related to a flaky test. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.16
Development

Successfully merging this pull request may close these issues.

Subtest junit parsing is fragile, indicates failures when stdout interleaves with go test output
5 participants
@liggitt @k8s-ci-robot @BenTheElder @cblecker @Huang-Wei