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

Limit Dependencies to the stdlib #1971

Closed
wants to merge 68 commits into from

Conversation

asahasrabuddhe
Copy link
Member

@asahasrabuddhe asahasrabuddhe commented Aug 22, 2024

What type of PR is this?

  • cleanup

What this PR does / why we need it:

This PR removes the last remaining dependency testify. In line with the famous Go Proverb: A little copying is better than a little dependency. I have decided to copy over the necessary code to make our tests work.

Which issue(s) this PR fixes:

Fixes #1890

Special notes for your reviewer:

(fill-in or delete this section)

Testing

(fill-in or delete this section)

Release Notes

(REQUIRED)


@asahasrabuddhe asahasrabuddhe changed the title 1890 limit dependencies to stdlib Limit Dependencies to the stdlib Aug 22, 2024
@asahasrabuddhe asahasrabuddhe marked this pull request as ready for review August 22, 2024 09:10
@asahasrabuddhe asahasrabuddhe requested a review from a team as a code owner August 22, 2024 09:10
internal/testing/testing.go Outdated Show resolved Hide resolved
internal/testing/testing.go Outdated Show resolved Hide resolved
internal/testing/testing.go Show resolved Hide resolved
@bartekpacia
Copy link
Member

As much as I like stdlib and desest testing frameworks like testify, I'm not sure it's the right course of action here.

@asahasrabuddhe
Copy link
Member Author

As much as I like stdlib and desest testing frameworks like testify, I'm not sure it's the right course of action here.

How do you think we should approach this then?

@bartekpacia
Copy link
Member

How do you think we should approach this then?

I don't know, honestly. I'll pass on reviewing this PR and just defer to other maintainers here.

That said, thanks a lot for contributing to this project :) It's great! I'm just not versed well-enough in this codebase.

Copy link
Member

@yassinebenaid yassinebenaid left a comment

Choose a reason for hiding this comment

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

I'm fan of perfectionism LOL

}

// NotContains asserts that the specified string does NOT contain the specified substring.
func NotContains(t *testing.T, s, contains string, msgAndArgs ...interface{}) bool {
Copy link
Member

Choose a reason for hiding this comment

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

I believe that grammar is important even for us developers 👨‍🏫

- func NotContains(t *testing.T, s, contains string, msgAndArgs ...interface{}) bool {
+ func DoesNotContain(t *testing.T, s, contains string, msgAndArgs ...interface{}) bool {


// RequireNotContains asserts that the specified string does NOT contain the specified substring. If it does, the test
// will stop.
func RequireNotContains(t *testing.T, s, contains string, msgAndArgs ...interface{}) {
Copy link
Member

Choose a reason for hiding this comment

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

- func RequireNotContains(t *testing.T, s, contains string, msgAndArgs ...interface{}) {
+ func RequireDoesNotContain(t *testing.T, s, contains string, msgAndArgs ...interface{}) {

@yassinebenaid
Copy link
Member

yassinebenaid commented Aug 22, 2024

I know this might sound weird. But, I think we should have a test for those testing functions too using the standard library only. These tests would test that those testing functions are actually doing the job of testing (term test is widely used here LOL).

for example:

func TestThatNilActuallyWorks(t *testing.T){
    if !Nil(nil){
        t.Fail("Nil doesn't actually work, our tests are risky then.")
    }
}

@abitrolly
Copy link
Contributor

You've forgot to include license.

@dearchap
Copy link
Contributor

Do we really want to maintain a testing framework ourselves ? We have to add tests for the test framework itself now. Is testify required by the users of this library to run their code ?

@bartekpacia
Copy link
Member

Is testify required by the users of this library to run their code ?

It's not, it doesn't appear in my app's go.mod and go.sum.

@dearchap
Copy link
Contributor

Then what's the point of us struggling so much for this one library

@bartekpacia
Copy link
Member

bartekpacia commented Aug 23, 2024

Yeah, I am with @dearchap here. Let's stay with testify.

There are better issues to spend time on in this codebase:)

@asahasrabuddhe
Copy link
Member Author

Agreed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Strictly limit default dependencies to stdlib
5 participants