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

filter go test output live with gotestsum #125534

Merged
merged 1 commit into from
Sep 5, 2024
Merged

filter go test output live with gotestsum #125534

merged 1 commit into from
Sep 5, 2024

Conversation

pohly
Copy link
Contributor

@pohly pohly commented Jun 16, 2024 •
edited
Loading

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Running go test -json in pull-kubernetes-unit and grepping the output does not work well and against the intent of the grep includes log output:

kubernetes/hack/make-rules/test.sh

Lines 148 to 149 in 9de747c

# Show only summary lines by matching lines like "status package/test"
go_test_grep_pattern="^[^[:space:]]\ [[:space:]]\ [^[:space:]]\ /[^[[:space:]]\ "

->

     [0615 07:10:24] Running tests without code coverage and with -race
{"Time":"2024-06-15T07:11:28.996778255Z","Action":"output","Package":"k8s.io/kubernetes/cluster/gce/cos","Test":"TestCreateMasterAuditPolicy","Output":"        /tmp/configure-helper-test338931032/kube-env: line 1: `}'\n"}
{"Time":"2024-06-15T07:11:30.17351168Z","Action":"output","Package":"k8s.io/kubernetes/cluster/gce/cos","Output":"ok  \tk8s.io/kubernetes/cluster/gce/cos\t2.941s\n"}

A better approach is to let gotestsum process filter the go test output while it is produced. Then we can control via -format what gotestsum prints to the console. The default is a short summary. With --format=standard-verbose (enabled by FULL_LOG) we get the normal go test -v output.

Special notes for your reviewer:

Our bash code for running go test is fairly complex, in particular when using coverage and invoking each test by itself. I briefly tried generating a bash script which then can be given to gotestsum --raw-command <script>, but getting the quoting right is tricky. The current solution is to not use a pipe and let xargs read from a temporary file instead.

Does this PR introduce a user-facing change?

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/dependency Issues or PRs related to dependency changes approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 16, 2024
@pohly pohly mentioned this pull request Jun 16, 2024
Open
pohly
pohly commented Jun 17, 2024
View reviewed changes
hack/make-rules/test.sh Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 22, 2024
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 31, 2024
pohly
pohly commented Jul 31, 2024
View reviewed changes
hack/make-rules/test.sh Outdated Show resolved Hide resolved
@pohly pohly changed the title WIP: filter go test output live with gotestsum filter go test output live with gotestsum Jul 31, 2024
@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, 2024
@pohly
Copy link
Contributor Author

pohly commented Jul 31, 2024

@dims: I removed the dependency on an unreleased gotestsum feature, so we could merge it now. But see #125534 (comment)...

A test run looks good: https://prow.k8s.io/log?container=test&id=1818617277273083904&job=pull-kubernetes-unit

Same number of tests as in other runs (1099). What stands out more now is the amount of logging that still goes to stderr. Contextual logging with per-test logging would get rid of all that.

/hold

Before we merge I should break one unit test temporarily to ensure that this gets recorded properly.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 31, 2024
@dims
Copy link
Member

dims commented Jul 31, 2024

/assign @BenTheElder

@pohly pohly marked this pull request as draft July 31, 2024 14:33
@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. sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 31, 2024
@pohly
Copy link
Contributor Author

pohly commented Jul 31, 2024

/test pull-kubernetes-unit

After breaking staging/src/k8s.io/dynamic-resource-allocation/cel/compile_test.go

@k8s-ci-robot k8s-ci-robot added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Aug 2, 2024
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 16, 2024
@pohly pohly force-pushed the gotestsum branch 2 times, most recently from 5cafa74 to f33eab4 Compare August 19, 2024 12:54
@pohly
Copy link
Contributor Author

pohly commented Aug 19, 2024

@aojea, @BenTheElder: I rebased. Okay to merge now?

@aojea
Copy link
Member

aojea commented Aug 19, 2024

the job failure seems related

pull-kubernetes-unit — Job failed.                     BaseSHA:8db6fc7e3fabc00f12be17d5a1623cba278d4b6b

hack/make-rules/test.sh: line 224: jsonfile: unbound variable

@pohly
test: filter "go test" output with gotestsum instead of grep
a7da865 
Filtering the output with grep leads to hard to read log output, e.g. from
pull-kubernetes-unit:

        [0613 15:32:48] Running tests without code coverage and with -race
    {"Time":"2024-06-13T15:33:47.845457374Z","Action":"output","Package":"k8s.io/kubernetes/cluster/gce/cos","Test":"TestCreateMasterAuditPolicy","Output":"        /tmp/configure-helper-test47992121/kube-env: line 1: `}'\n"}
    {"Time":"2024-06-13T15:33:49.053732803Z","Action":"output","Package":"k8s.io/kubernetes/cluster/gce/cos","Output":"ok  \tk8s.io/kubernetes/cluster/gce/cos\t2.906s\n"}

We can do better than that. When feeding the output of the "go test" command(s)
into gotestsum *while it runs*, we can use --format=standard-quiet (= normal go
test output) or --format=standard-verbose (= `go test -v`) when FULL_LOG is
requested to get nicer output.

This works when testing everything at once. This was said to be not possible
when doing coverage profiling. But recent Go no longer has that limitation, so
the xargs trick gets removed. All that we need to do for coverage profiling is
to add some additional parameters and the conversion to HTML.
@pohly
Copy link
Contributor Author

pohly commented Aug 19, 2024

hack/make-rules/test.sh: line 224: jsonfile: unbound variable

Indeed. I need to initialize the variable when not entering the if clause. Fixed.

@pohly pohly mentioned this pull request Aug 19, 2024
Closed
@pohly
Copy link
Contributor Author

pohly commented Aug 19, 2024

Tests are passing now.

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 20, 2024
@aojea
Copy link
Member

aojea commented Aug 21, 2024

LGTM
/assign @BenTheElder @liggitt

for final approval

@dims
Copy link
Member

dims commented Sep 5, 2024

thanks @aojea

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 5, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 3d061c7366ab57206c943cee332a0da3bef046cc

@pohly
Copy link
Contributor Author

pohly commented Sep 5, 2024

/hold cancel

I think this had enough reviews. Let's merge it.

@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 Sep 5, 2024
BenTheElder
BenTheElder reviewed Sep 5, 2024
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

@BenTheElder
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, dims, pohly

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 merged commit 2049360 into kubernetes:master Sep 5, 2024
14 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.32 milestone Sep 5, 2024
This was referenced Sep 10, 2024
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aojea aojea aojea left review comments

@BenTheElder BenTheElder BenTheElder left review comments

@dims dims dims left review comments

@mikedanese mikedanese Awaiting requested review from mikedanese

@SataQiu SataQiu Awaiting requested review from SataQiu

@liggitt liggitt Awaiting requested review from liggitt

Assignees

@dims dims

@BenTheElder BenTheElder

@liggitt liggitt

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 cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note-none Denotes a PR that doesn't merit a release note. sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Node: code and documentation PRs
Archived in project
Milestone
v1.32
Development

Successfully merging this pull request may close these issues.
7 participants
@pohly @dims @BenTheElder @bart0sh @aojea @k8s-ci-robot @liggitt