Ginkgo update + stack fix #82176

pohly · 2019-08-30T14:30:12Z

What type of PR is this?
/kind cleanup

What this PR does / why we need it:

After merging
259bb3b#diff-eb7b79470992813ea1905e96c298b47b
ExpectEqual and some of the other wrappers logged the failure twice,
once inside the wrapper itself and once in the failure handler.

Logging the stack backtrace is useful because many assertions still
don't contain an explanation and therefore knowing where they occur is
crucial. Now all failures are logged with a "Full Stack Trace", not
just those with a wrapper. The stack is pruned to skip over wrapper
functions and removes Ginkgo internal functions to keep the stack
trace smaller.

Failures occurring in the wrappers were recorded as occurring in those
wrappers. Now the wrappers are skipped and the caller is recorded
instead.

Fixes: #82013

Does this PR introduce a user-facing change?:

more complete and accurate logging of stack backtraces in E2E failures

pohly · 2019-08-30T14:30:50Z

This is WIP because Ginkgo 1.10.0 still isn't quite working correctly: onsi/ginkgo#600

pohly · 2019-08-30T18:29:23Z

/retest

oomichi · 2019-08-30T19:19:42Z

test/e2e/framework/log/logger.go

@@ -48,8 +51,9 @@ func Failf(format string, args ...interface{}) {
 // (for example, for call chain f -> g -> FailfWithOffset(1, ...) error would be logged for "f").


nit: nice to add Stack Trace also in this explanation.

oomichi · 2019-08-30T19:26:03Z

test/e2e/framework/log/logger.go

@@ -19,6 +19,9 @@ package log

 import (


@pohly This e2elog package will be removed as line 17.
Could you apply this change to test/e2e/framework/log.go ?
I did mistake to forget copying the unit test to test/e2e/framework/log_test.go with #81426
Sorry about that.

I see. I've moved it and also some of the util.go functions that are only used in log.go. I also updated the comment in framework/e2e/log/logger.go to be more explicit.

oomichi · 2019-08-30T20:09:19Z

test/e2e/framework/log/logger.go

+// entries coming from Ginkgo.
+//
+// This is a copy of https://github.com/onsi/ginkgo/blob/f90f37d87fa6b1dd9625e2b1e83c23ffae3de228/internal/codelocation/code_location.go#L25
+func PrunedStack(skip int) string {


The original function name is PruneStack in ginkgo. I guess this name (with d) is intentional, right?
(I tried to search this name in ginkgo repo, and I missed that with the name)

I intentionally renamed it, because this version is simpler and returns the "pruned stack" instead of taking an arbitrary string and pruning it.

I added "This is a modified copy of PruneStack in ..."

oomichi · 2019-08-30T20:10:20Z

/cc @oomichi

timothysc

/approve

timothysc · 2019-10-03T15:27:18Z

/assign @liggitt
^ requires approval for dep changes.

liggitt · 2019-10-03T15:40:36Z

/approve

k8s-ci-robot · 2019-10-03T15:41:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, oomichi, pohly, timothysc

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:

~~OWNERS~~ [liggitt]
~~staging/src/k8s.io/api/OWNERS~~ [liggitt]

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

alejandrox1 · 2019-10-04T18:59:36Z

@pohly could you try doing a git commit --amend && git push -f origin ginkgo-stack-fix to try and remove the needs-rebase label ? 🤔

cblecker · 2019-10-04T19:10:15Z

/test all

Updated to keep both Ginkgo and Gomega at the latest releases.

pohly · 2019-10-04T19:40:33Z

could you try doing a git commit --amend && git push -f origin ginkgo-stack-fix to try and remove the needs-rebase label ?

Done. @alejandrox1 now you need to LGTM again.

oomichi · 2019-10-04T22:49:52Z

/lgtm

fejta-bot · 2019-10-05T04:47:09Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

Ginkgo update + stack fix Kubernetes-commit: b140b43

k8s-ci-robot requested review from brendandburns, caesarxuchao and a team August 30, 2019 14:32

pohly mentioned this pull request Aug 30, 2019

stack backtrace: fix skipping onsi/ginkgo#600

Merged

pohly changed the title ~~WIP: Ginkgo update + stack fix~~ Ginkgo update + stack fix Aug 30, 2019

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 Aug 30, 2019

oomichi reviewed Aug 30, 2019

View reviewed changes

k8s-ci-robot requested a review from oomichi August 30, 2019 20:10

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed area/cloudprovider area/apiserver do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Oct 1, 2019

timothysc approved these changes Oct 3, 2019

View reviewed changes

k8s-ci-robot assigned liggitt Oct 3, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 3, 2019

oomichi mentioned this pull request Oct 4, 2019

Refactored e2e-test-framework util.go #83510

Merged

alejandrox1 mentioned this pull request Oct 4, 2019

Refactored e2e-test-framework logging #83512

Closed

cblecker removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 4, 2019

vendor: update gomega to v1.7.0

27b9a9c

Updated to keep both Ginkgo and Gomega at the latest releases.

pohly force-pushed the ginkgo-stack-fix branch from 84ea6c0 to 27b9a9c Compare October 4, 2019 19:39

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 4, 2019

k8s-ci-robot merged commit b140b43 into kubernetes:master Oct 5, 2019

wking pushed a commit to wking/kubernetes that referenced this pull request Jul 21, 2020

Merge pull request kubernetes#82176 from pohly/ginkgo-stack-fix

987b623

Ginkgo update + stack fix Kubernetes-commit: b140b43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ginkgo update + stack fix #82176

Ginkgo update + stack fix #82176

pohly commented Aug 30, 2019 •

edited

pohly commented Aug 30, 2019

pohly commented Aug 30, 2019

oomichi Aug 30, 2019

pohly Sep 2, 2019

oomichi Aug 30, 2019

pohly Sep 2, 2019

oomichi Aug 30, 2019

pohly Sep 2, 2019

pohly Sep 2, 2019

oomichi commented Aug 30, 2019

timothysc left a comment

timothysc commented Oct 3, 2019

liggitt commented Oct 3, 2019

k8s-ci-robot commented Oct 3, 2019

alejandrox1 commented Oct 4, 2019

cblecker commented Oct 4, 2019

pohly commented Oct 4, 2019

oomichi commented Oct 4, 2019

fejta-bot commented Oct 5, 2019

		@@ -48,8 +51,9 @@ func Failf(format string, args ...interface{}) {
		// (for example, for call chain f -> g -> FailfWithOffset(1, ...) error would be logged for "f").

Ginkgo update + stack fix #82176

Ginkgo update + stack fix #82176

Conversation

pohly commented Aug 30, 2019 • edited

pohly commented Aug 30, 2019

pohly commented Aug 30, 2019

oomichi Aug 30, 2019

Choose a reason for hiding this comment

pohly Sep 2, 2019

Choose a reason for hiding this comment

oomichi Aug 30, 2019

Choose a reason for hiding this comment

pohly Sep 2, 2019

Choose a reason for hiding this comment

oomichi Aug 30, 2019

Choose a reason for hiding this comment

pohly Sep 2, 2019

Choose a reason for hiding this comment

pohly Sep 2, 2019

Choose a reason for hiding this comment

oomichi commented Aug 30, 2019

timothysc left a comment

Choose a reason for hiding this comment

timothysc commented Oct 3, 2019

liggitt commented Oct 3, 2019

k8s-ci-robot commented Oct 3, 2019

alejandrox1 commented Oct 4, 2019

cblecker commented Oct 4, 2019

pohly commented Oct 4, 2019

oomichi commented Oct 4, 2019

fejta-bot commented Oct 5, 2019

pohly commented Aug 30, 2019 •

edited