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

util/must: convenience wrapper for making many assertions #107418

Closed
erikgrinaker opened this issue Jul 22, 2023 · 5 comments
Closed

util/must: convenience wrapper for making many assertions #107418

erikgrinaker opened this issue Jul 22, 2023 · 5 comments
Assignees
Labels
A-testing Testing tools and infrastructure C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-testeng TestEng Team

Comments

@erikgrinaker
Copy link
Contributor

erikgrinaker commented Jul 22, 2023

See #106508.

It's common to execute many assertions at once, see e.g. storage.assertMVCCIteratorInvariants(). Returning errors from each individual assertion can get tedious and clutter the code. It would be convenient to run many assertions in a block, if we can do so efficiently.

Here's an example of how this might look:

err := must.With(ctx, func(m *must.Must) {
  m.True(true, "yep")
  m.False(false, "again")
  value, err := someFunc()
  m.NoError(err, "someFunc failed")
  m.Equal(value, "foo", "someFunc returned invalid value")
})

This would be implemented by throwing panics on assertion failures, and recovering the panic in must.With. A specific panic value would be thrown, wrapping the assertion error, and only this value would be recovered -- other panics would be propagated.

must.With itself would handle the assertion failure like any other assertion failure, i.e. fatal or propagate.

Jira issue: CRDB-30031

@erikgrinaker erikgrinaker added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-testing Testing tools and infrastructure labels Jul 22, 2023
@erikgrinaker erikgrinaker self-assigned this Jul 22, 2023
@erikgrinaker erikgrinaker added the T-testeng TestEng Team label Jul 23, 2023
@blathers-crl
Copy link

blathers-crl bot commented Jul 23, 2023

cc @cockroachdb/test-eng

@erikgrinaker
Copy link
Contributor Author

Immediately hit this limitation ☹️

method must have no type parameters

@erikgrinaker
Copy link
Contributor Author

erikgrinaker commented Jul 25, 2023

A couple of other options:

  1. Pass a function that handles errors for regular assertions, promoting them to panics that are then recovered . It's a bit weird though, and arguably doesn't help readability all that much.

    err := must.With(func(try must.Try) {
      try(must.True(ctx, true, "yep"))
      try(must.False(ctx, false, "again"))
      value, err := someFunc()
      try(must.NoError(ctx, err, "someFunc failed"))
      try(m.Equal(ctx, value, "foo", "someFunc returned invalid value"))
    })
  2. Pass a value in the context that triggers the panics. It's fairly clean, but the functions still return an error variable that will never be set to something non-nil, so it should always be ignored. We also want the errcheck linter to ignore these blocks. This muddies the water a bit, since we need to handle the returned errors in some contexts but not others.

    err := must.With(ctx context.Context, func(ctx context.Context) {
      must.True(ctx, true, "yep")
      must.False(ctx, false, "again")
      value, err := someFunc()
      must.NoError(ctx, err, "someFunc failed")
      must.Equal(ctx, value, "foo", "someFunc returned invalid value")
    })

@erikgrinaker
Copy link
Contributor Author

I've opened a prototype draft PR in #107790 that uses panics to propagate assertion failures, i.e. option 2 above. It's not ideal, but it's the best I've come up with so far.

Curious if you have any ideas or thoughts here @knz.

@erikgrinaker
Copy link
Contributor Author

must is no more, see #108272.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testing Testing tools and infrastructure C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-testeng TestEng Team
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

1 participant