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

Remove assert.New and require.New #2657

Merged
merged 1 commit into from
Apr 25, 2019
Merged

Conversation

lkowalick
Copy link
Contributor

@lkowalick lkowalick commented Apr 24, 2019

Remove assert.New and require.New from the codebase.

@ZenGround0
Copy link
Contributor

I don't like it but will not block.

@travisperson
Copy link
Contributor

Whats the reason for not wanting to use assert.New and require.New?

@acruikshank
Copy link
Contributor

acruikshank commented Apr 25, 2019

Here are a few justifications for this PR:

  1. Defining a variable that shadows the package is bad practice.
  2. using assert.New and require.New is adding hundreds of lines to our test code.
  3. We're doing it wrong in many of our tests. If you include a t.Run within a test you must define new assert and require for a new value of t, but we often use the assert and require in closure scope. Using it incorrectly makes our tests unsafe under parallelization (I fixed a lot of this in our flaky test phase). Using these correctly adds a lot of boilerplate when you're trying to write a lot of quick, idiomatic, well documented test cases off an existing setup.
  4. The people that established this convention are no longer on the project. I was never given any explanation for why we standardized on the OO versions of this code. If we default to the functional style, there will be nothing wrong with using the OO style occasionally (though I challenge you to provide a reason beyond the extra 3 characters in the assertions).
  5. It's tedious. Writing a test you start by adding the assert.New, then you start writing your test and realize you need a require, so you go back up and add the require.New, then you realize that you want all requires so you have to go up and delete the assert.New. It's a really small amount of effort, but it's a small amount of effort multiplied over a whole lot of testing. This is a great place to reduce friction.

This is something I've been wanting to propose for a long time.

@travisperson
Copy link
Contributor

Okay sounds good. @rkowalick can you update the title and description?

@lkowalick lkowalick changed the title WIP (is this worth it?) WIP Remove assert.New and require.Net Apr 25, 2019
@frrist frrist changed the title WIP Remove assert.New and require.Net WIP Remove assert.New and require.New Apr 25, 2019
@lkowalick lkowalick requested a review from acruikshank April 25, 2019 21:21
@lkowalick lkowalick force-pushed the chore/remove-require-assert-new branch from cf84e65 to c633b90 Compare April 25, 2019 21:21
@lkowalick lkowalick force-pushed the chore/remove-require-assert-new branch from c633b90 to ac09f23 Compare April 25, 2019 21:28
@lkowalick lkowalick requested a review from shannonwells April 25, 2019 21:34
Copy link
Contributor

@shannonwells shannonwells 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 a fan. Having tried it, it doesn't help much to rename the import to something else (I've used ast and req). In my current branch where I'm also (because of this PR) replacing everything by just calling <assert|require>.NoError(t, err) explicitly, it makes for cleaner code with fewer lines, and also, I don't create requires just to send to mustDoAThing type functions.

@lkowalick lkowalick changed the title WIP Remove assert.New and require.New Remove assert.New and require.New Apr 25, 2019
Copy link
Contributor

@acruikshank acruikshank left a comment

Choose a reason for hiding this comment

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

Little green 't, 's will be floating past my vision as I close my eyes tonight, but it all looks in order.

@lkowalick lkowalick merged commit caab72e into master Apr 25, 2019
@lkowalick lkowalick deleted the chore/remove-require-assert-new branch April 25, 2019 22:01
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.

5 participants