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

Proposal: changes for next major release (v4) #257

Open
dnephin opened this issue Feb 25, 2023 · 4 comments
Open

Proposal: changes for next major release (v4) #257

dnephin opened this issue Feb 25, 2023 · 4 comments

Comments

@dnephin
Copy link
Member

dnephin commented Feb 25, 2023

With the opportunity to use generics to improve the assert package (#255), we may want to release a new major version. This issue is to discuss and track changes that would require a major version bump, so that we can include all of them when the time is right.

  1. Change the signature of the assert, and assert/cmp functions to use generics instead of any as the argument types (assert: use type constraints (generics) #255)

  2. Combine the skip, env, and poll packages into a single new package. These packages are very small, and combining them into a single package should make it easier to use gotest.tools by requiring fewer imports. I'm not sure what to name that package yet. They could be included as part of the assert package, but the names don't read as well.

  3. Replace the golden package with a GoldenFile(t, filename) function in the assert package. assert: golden variables #237 added support for golden-like behaviour using expected values stored in a Go variable. We should be able to do something similar:

    func GoldenFile(t TestingT, filename string) string { ... }

    Similar to golden variables, when called with -update, assert.Assert should be able to detect that the expected argument is a GoldenFile and use the actual value to update the specified filename.

  4. Maybe icmd could change to use context.Context to set the timeout instead of a time.Duration passed to WaitOnCommand ? Some way to pass a deadline/timeout using context.Context might be good.

Are there other breaking changes we should consider? Anything in fs that could change now that the stdlib has an fs package or because of generics?

@fgimian
Copy link

fgimian commented Apr 20, 2023

Just my 2 cents below 😄

1. Change the signature of the `assert`, and `assert/cmp` functions to use generics instead of `any` as the argument types ([assert: use type constraints (generics) #255](https://github.com/gotestyourself/gotest.tools/pull/255))

👍

2. Combine the `skip`, `env`, and `poll`  packages into a single new package. These packages are very small, and combining them into a single package should make it easier to use `gotest.tools` by requiring fewer imports. I'm not sure what to name that package yet.  They could be included as part of the `assert` package, but the names don't read as well.

I would suggest not including them in the assert package, one thing I like about this library is how simple and focused the assert package is. It's easy to see everything you can assert at a glance. Adding other utilities in that package will probably ruin that a bit.

Perhaps the package could just be called test? Not sure about a better name TBH😄

3. Replace the `golden` package with a `GoldenFile(t, filename)` function in the `assert` package. [assert: golden variables #237](https://github.com/gotestyourself/gotest.tools/pull/237) added support for golden-like behaviour using expected values stored in a Go variable.

👍

4. Maybe `icmd` could change to use `context.Context` to set the timeout instead of a `time.Duration` passed to `WaitOnCommand` ? Some way to pass a deadline/timeout using `context.Context` might be good.

👍

Are there other breaking changes we should consider? Anything in fs that could change now that the stdlib has an fs package or because of generics?

It may be nice to have colored output as an option, similar to got. This looks really nice when running with gotestsum.

e.g.

image

Similarly with the struct diffs too 😄

I do also wonder if the assert package should offer a few more asserts, but I realise that's a slippery slope.

One more little thing I'd change (I found this confusing), is not to alias cmp to is anywhere in the code or documentation. I think this just causes more confusion as there is another test framework called is.

@dnephin
Copy link
Member Author

dnephin commented Apr 22, 2023

Thank you for the feedback! I like the idea of using color to improve the visibility of the failure messages. With type constraints I think we'll be able to remove the type from the failure message text (Ex: int, vs int64), which should also help make the failure messages more concise.

Removing the aliasing of the cmp package is also a good idea. I'll open a separate issue for that. I think we can make that change without it being a breaking change. #260

I do also wonder if the assert package should offer a few more asserts

I've been trying to reduce the number of assert functions as much as possible. I think the ones we have now cover at least 95% of assertions. For the remaining few percent I think documenting patterns for using assert.Assert will be better than adding more assert functions.

@dolmen
Copy link
Contributor

dolmen commented Jun 19, 2023

See also #266 (define a separate Go module for assert/cmd/gty-migrate-from-testify) (however I think that this doesn't need to wait for v4)

@milas
Copy link

milas commented Jul 7, 2023

4. Maybe icmd could change to use context.Context to set the timeout instead of a time.Duration passed to WaitOnCommand ? Some way to pass a deadline/timeout using context.Context might be good.

Having the ability to pass in your own context would also be helpful (a la https://pkg.go.dev/os/exec#CommandContext).

As a use case, it's handy to do the following:

ctx, cancel := context.WithCancel(context.Background())
t.Cleanup(cancel)

...

That way, any icmd instances running when the test ends would be killed.

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

No branches or pull requests

4 participants