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

testutil: use TestingTB instead of testing.TB #8647

Merged
merged 1 commit into from
Sep 10, 2020
Merged

testutil: use TestingTB instead of testing.TB #8647

merged 1 commit into from
Sep 10, 2020

Conversation

paulburlumi
Copy link
Contributor

These changes were made as a result of a suggestion by @dnephin in #7559.
This PR enables using a testing.TB like interface (e.g. GinkgoT) in addition to the 'real' testing.TB interface.
See onsi/ginkgo#582 for further details.

cc @dnephin

@hashicorp-cla
Copy link

hashicorp-cla commented Sep 10, 2020 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

mkeeler
mkeeler approved these changes Sep 10, 2020
View reviewed changes
Copy link
Member

@mkeeler mkeeler left a comment

Choose a reason for hiding this comment

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

LGTM thanks for the PR

mkeeler
mkeeler requested changes Sep 10, 2020
View reviewed changes
Copy link
Member

@mkeeler mkeeler left a comment

Choose a reason for hiding this comment

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

I looked at the CI results and the build failures are not flakes. Looks like one func needs removing from the TestingTB interface.

sdk/testutil/types.go Outdated Show resolved Hide resolved
@paulburlumi
Copy link
Contributor Author

@mkeeler I've fixed a lint issue, but now the integration envoy tests appear to be failing with an error to fetch the alpine container. I don't think it is related to my changes.

dnephin
dnephin reviewed Sep 10, 2020
View reviewed changes
Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

Thank you for making this change! I've left one suggestion below.

There are a few other functions in this package which use testing.T. It would probably be good to convert more of them to this new interface, but we don't want to do it all at once.

sdk/testutil/types.go Outdated Show resolved Hide resolved
@dnephin
Copy link
Contributor

dnephin commented Sep 10, 2020

Yes, the integration test failure is unrelated

dnephin
dnephin approved these changes Sep 10, 2020
View reviewed changes
Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

Thank you for making this change! LGTM

The test failure is a known flake, not related to this change.

@dnephin dnephin merged commit f51276e into hashicorp:master Sep 10, 2020
@paulburlumi paulburlumi deleted the dev/testutil_testing_interface branch September 10, 2020 21:13
@paulburlumi paulburlumi mentioned this pull request Sep 10, 2020
Closed
@dnephin dnephin mentioned this pull request Sep 11, 2020
Closed
@c0ffee c0ffee mentioned this pull request Aug 16, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkeeler mkeeler mkeeler requested changes

@dnephin dnephin dnephin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@paulburlumi @hashicorp-cla @dnephin @mkeeler