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

Adding TestInstallation.AssertionsT #10600

Merged
merged 2 commits into from
Feb 7, 2025
Merged

Adding TestInstallation.AssertionsT #10600

merged 2 commits into from
Feb 7, 2025

Conversation

ryanrolds
Copy link

@ryanrolds ryanrolds commented Feb 7, 2025

Description

While looking into a flakey test I noticed that trying to check if the test failed in the AfterTest method wasn't working as expected; The Failed method was reporting false when there was a failure, preventing me from making a test-level cluster dump in the _test artifacts.

It turns out that TestInstallation.Assertions is created when the suite runs and uses the suite's testing.T. That leads to the issue and the framework calls it out in the log with testing.go:1577: test executed panic(nil) or runtime.Goexit: subtest may have called FailNow on a parent test.

This PR introduces a new method that takes the current testing.T and returns an appropriately scoped assertions provider. The previous method has been flagged in a way that VSC (and likely other IDEs) will call it out.

After we finish abating this issue, we should be able implement test-level dumps across our kubernetes/e2e tests. Making test failures easier to debug.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

@ryanrolds ryanrolds changed the title Adding TestInstrallation.AssertionsT Adding TestInstallation.AssertionsT Feb 7, 2025
@ryanrolds ryanrolds marked this pull request as ready for review February 7, 2025 18:01
@ryanrolds
Copy link
Author

/kick-ci

Copy link

github-actions bot commented Feb 7, 2025

Visit the preview URL for this PR (updated for commit be68573):

https://gloo-edge--pr10600-rolds-assertiont-lervclao.web.app

(expires Fri, 14 Feb 2025 18:55:04 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 77c2b86e287749579b7ff9cadb81e099042ef677

Copy link

@jmcguire98 jmcguire98 left a comment

Choose a reason for hiding this comment

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

LGTM, nice catch!

@ryanrolds
Copy link
Author

/kick-ci

@soloio-bulldozer soloio-bulldozer bot merged commit 2d3e0fc into main Feb 7, 2025
20 checks passed
@soloio-bulldozer soloio-bulldozer bot deleted the rolds/assertiont branch February 7, 2025 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants