Skip to content

test/e2e/funcs: add helpful error context to ApplyResources and others, vs. features.go:94#5722

Closed
sttts wants to merge 1 commit into
crossplane:mainfrom
sttts:sttts-e2e-applyresources-context
Closed

test/e2e/funcs: add helpful error context to ApplyResources and others, vs. features.go:94#5722
sttts wants to merge 1 commit into
crossplane:mainfrom
sttts:sttts-e2e-applyresources-context

Conversation

@sttts

@sttts sttts commented May 21, 2024

Copy link
Copy Markdown
Contributor

Description of your changes

When these helpers error, the parent source file and line is something like

feature.go:194: resources not deleted: client rate limiter Wait returned an error: rate: Wait(n=1) would exceed context deadline:

because the returned closure is called from feature.go. To get a helpful caller context, we have to pass it into the closure from the constructor.

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • [ ] Added or updated unit tests.
  • [ ] Added or updated e2e tests.
  • [ ] Linked a PR or a docs tracking issue to document this change.
  • [ ] Added backport release-x.y labels to auto-backport this PR.

Need help with this checklist? See the cheat sheet.

Dr. Stefan Schimanski
test/e2e/funcs: add helpful error context to ApplyResources and other…
c4a8d72 
…s, vs. features.go:94

Signed-off-by: Dr. Stefan Schimanski <[email protected]>
@sttts
sttts requested a review from a team as a code owner May 21, 2024 12:33
@sttts
sttts requested a review from phisco May 21, 2024 12:33
phisco
phisco approved these changes May 21, 2024
View reviewed changes

@phisco phisco left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

unfortunate we have to do this, but I couldn't find a better way

@negz

negz commented May 21, 2024

Copy link
Copy Markdown
Member

unfortunate we have to do this, but I couldn't find a better way

I think github.com/pkg/errors used to automatically decorate errors with this context, but it's now deprecated. 🤔

It looks like it was doing something similar to what @sttts is doing here - https://github.com/pkg/errors/blob/master/stack.go#L163

I wonder whether there would be value in adding this to crossplane-runtime/pkg/errors? We could either add context to all wrapped errors, or have an optional WithCaller wrapper?

@jbw976 jbw976 mentioned this pull request May 21, 2024
Open
17 tasks
@negz

negz commented Jun 4, 2024

Copy link
Copy Markdown
Member

I tried to find a less hacky feeling way to address this and ended up with kubernetes-sigs/e2e-framework#421.

It ends up looking like this:

                +e2e | I0603 22:29:59.923378     255 environment.go:76] "Setting test suite" value="base"
                +e2e | === RUN   TestCompositionMinimal/TestCompositionMinimal/CreateClaim
                +e2e |     apiextensions_test.go:79: Applied resources from test/e2e/manifests/apiextensions/composition/minimal/claim.yaml (matched 1 manifests)
                +e2e |     apiextensions_test.go:79: Waiting 30s for NopResource.nop.example.org default/apiextensions-composition-minimal to exist...
                +e2e |     feature.go:965: - CREATED:   NopResource.nop.example.org default/apiextensions-composition-minimal (2024-06-03 22:31:54 +0000 UTC)
                +e2e |     apiextensions_test.go:79: 1 resources found to exist after 0.503s
                +e2e |     feature.go:979: - CONDITION: NopResource.nop.example.org default/apiextensions-composition-minimal: Synced=True Reason=ReconcileSuccess: "" (2024-06-03 22:31:54 +0000 UTC)
                +e2e |     feature.go:979: - CONDITION: NopResource.nop.example.org default/apiextensions-composition-minimal: Ready=False Reason=Waiting: Claim is waiting for composite resource to become Ready (2024-06-03 22:31:54 +0000 UTC)
                +e2e |     apiextensions_test.go:79: Waiting 2s for NopResource.nop.example.org default/apiextensions-composition-minimal to become Available...
                +e2e |     feature.go:965: - CREATED:   NopResource.nop.crossplane.io apiextensions-composition-minimal-r922g-bc7kj (2024-06-03 22:31:54 +0000 UTC)
                +e2e |     feature.go:965: - CREATED:   XNopResource.nop.example.org apiextensions-composition-minimal-r922g (2024-06-03 22:31:54 +0000 UTC)
                +e2e |     feature.go:979: - CONDITION: NopResource.nop.crossplane.io apiextensions-composition-minimal-r922g-bc7kj: Synced=True Reason=ReconcileSuccess: "" (2024-06-03 22:31:54 +0000 UTC)
                +e2e |     feature.go:979: - CONDITION: NopResource.nop.crossplane.io apiextensions-composition-minimal-r922g-bc7kj: Ready=False Reason=: "" (2024-06-03 22:31:54 +0000 UTC)
                +e2e |     feature.go:979: - CONDITION: XNopResource.nop.example.org apiextensions-composition-minimal-r922g: Ready=False Reason=Creating: Unready resources: nop-resource-1 (2024-06-03 22:31:54 +0000 UTC)
                +e2e |     feature.go:979: - CONDITION: XNopResource.nop.example.org apiextensions-composition-minimal-r922g: Synced=True Reason=ReconcileSuccess: "" (2024-06-03 22:31:54 +0000 UTC)
                +e2e |     feature.go:277: - CONDITION: NopResource.nop.example.org default/apiextensions-composition-minimal: Ready=False Reason=Waiting: Claim is waiting for composite resource to become Ready (2024-06-03 22:31:54 +0000 UTC)
                +e2e | W0603 22:31:57.049647     255 warnings.go:70] v1 ComponentStatus is deprecated in v1.19+
                +e2e |     feature.go:979: - CONDITION: NopResource.nop.crossplane.io apiextensions-composition-minimal-r922g-bc7kj: Ready=True Reason=: "" (2024-06-03 22:32:04 +0000 UTC)
                +e2e |     feature.go:979: - CONDITION: NopResource.nop.example.org default/apiextensions-composition-minimal: Ready=True Reason=Available: "" (2024-06-03 22:32:07 +0000 UTC)
                +e2e |     feature.go:979: - CONDITION: XNopResource.nop.example.org apiextensions-composition-minimal-r922g: Ready=True Reason=Available: "" (2024-06-03 22:32:07 +0000 UTC)
                +e2e | W0603 22:32:09.839090     255 warnings.go:70] ControllerConfig.pkg.crossplane.io/v1alpha1 is deprecated. Use DeploymentRuntimeConfig from pkg.crossplane.io/v1beta1 instead.
                +e2e |     apiextensions_test.go:79: resources did not have desired conditions: Available: client rate limiter Wait returned an error: rate: Wait(n=1) would exceed context deadline:

It's not perfect. You'll always see the environment.Test call point (e.g. apiextensions_test.go:79) in the log lines, not the specific step function call. I also couldn't get something working for the CONDITION log lines specifically, because they're emitted by a function called by another function without a *testing.T)

@sttts sttts mentioned this pull request Jun 4, 2024
Merged
@github-actions

github-actions Bot commented Sep 3, 2024

Copy link
Copy Markdown

Crossplane does not currently have enough maintainers to address every issue and pull request. This pull request has been automatically marked as stale because it has had no activity in the last 90 days. It will be closed in 14 days if no further activity occurs. Adding a comment starting with /fresh will mark this PR as not stale.

@github-actions github-actions Bot added stale and removed stale labels Sep 3, 2024
@github-actions

github-actions Bot commented Dec 3, 2024

Copy link
Copy Markdown

Crossplane does not currently have enough maintainers to address every issue and pull request. This pull request has been automatically marked as stale because it has had no activity in the last 90 days. It will be closed in 14 days if no further activity occurs. Adding a comment starting with /fresh will mark this PR as not stale.

@github-actions github-actions Bot added the stale label Dec 3, 2024
@jbw976

jbw976 commented Dec 4, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Looks like this was maybe addressed within e2e-framework by @negz in kubernetes-sigs/e2e-framework#421. That was included in https://github.com/kubernetes-sigs/e2e-framework/releases/tag/v0.5.0, so I think we could just need to bump our sigs.k8s.io/e2e-framework dependency to v0.5.0 and start getting the benefits 😇

Added that as a task to:

@jbw976

jbw976 commented Dec 11, 2024

Copy link
Copy Markdown
Member

After bumping locally to sigs.k8s.io/e2e-framework v0.5.0, I do see some improvements, e.g. what used to look like:

env.go:444: Waiting 30s for Composition.apiextensions.crossplane.io xnopresources.nop.example.org to exist...

Now looks like:

apiextensions_test.go:297: Waiting 30s for Composition.apiextensions.crossplane.io xnopresources.nop.example.org to exist...

So that type of check is more specific to the origin within the test case as opposed to within the e2e-framework internals. There are others that show from within crossplane test helpers like feature.go, but the bump to v0.5.0 is definitely an improvement nonetheless 😉

@github-actions github-actions Bot removed the stale label Feb 13, 2025
@github-actions

github-actions Bot commented May 14, 2025

Copy link
Copy Markdown

Crossplane does not currently have enough maintainers to address every issue and pull request. This pull request has been automatically marked as stale because it has had no activity in the last 90 days. It will be closed in 14 days if no further activity occurs. Adding a comment starting with /fresh will mark this PR as not stale.

@github-actions github-actions Bot added the stale label May 14, 2025
@github-actions github-actions Bot closed this May 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@phisco phisco phisco approved these changes

Assignees

No one assigned

Labels

stale

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sttts @negz @jbw976 @phisco