Expose "require" suite

[Blquinn]

I and many others expect / would prefer for the default behavior of the suite.Suite's .Equals() et al. methods to use the require behavior. See https://github.com/stretchr/testify/issues/599 for some context.

It would be nice if suite exposed a suite.RequireSuite, which could use the require.Assertions by default.

I left a comment in 599 outlining this struct. I believe that it should work just like the normal suite otherwise.

[boyan-soubachov]

Does getting the 'require' context through suite.Require() not solve your problem?

[Blquinn]

That definitely works, however this would be more of a convenience feature. Instead of doing r := s.require() and using the r in every test, this way, we'd simply define a separate struct, which would use that as the default behavior across all tests. I've found in my experience I almost never want a test to continue after the first failure occurs and this leads to either panics, or tons of noise, making it harder to find what's broken in the test's output.

[boyan-soubachov]

Funnily enough, I think suite should be unbiased towards assert or require. We don't seem to have a default bias in the normal libraries as one has to explicitly import assert and require. In my opinion it would therefore make sense to not have a bias in suite either.

I would be more interested in not implicitly including the assert.Assertions struct in the Suite struct and rather change it from:

type Suite struct {
    *assert.Assertions
    require *require.Assertions
    t       *testing.T
}

to

type Suite struct {
    assert *assert.Assertions
    require *require.Assertions
    t       *testing.T
}

That way, when suite testing is needed, the user would use:

func (s *MySuite) TestSomethingUsingSuite() {
    s.require().Equal(1,2)
    s.assert().Contains("Long text", "text")
}

It would definitely be a breaking change, so a v2 proposal.

I would really like to hear your thoughts, @Blquinn . @glesica , do you have any opinions on this?

[Blquinn]

I prefer explicitness in general, so I do like your idea. I also, however, like the conciseness of calling s.Equal(..) etc. I don't think we necessarily have to give up either in this case. Totally spitballing here, but what if we combine both techniques and optionally set which testing facility to use when we initialize the suite?

type Suite struct {
    defaultAssertions assertionsInterface // would be nil unless specified
    assert *assert.Assertions
    require *require.Assertions
    t *testing.T
}

// Implement assertionsInterface on suite

Then you'd add the assert/require interface to the Suite itself and call the facility based on whatever is set in the suite (if any). Not setting the value explicitly would return an error upon calling those methods.

[boyan-soubachov]

I see your point. Would the suite not be storing some form of global state which the user would have to switch between everytime they wanted to change from assert to require and vice versa?

[Blquinn]

I'd hesitate to call it global state because it would be per suite not a global variable somewhere. You'd be right that you'd be setting the default for that entire suite though. That seems reasonable to me, if you for instance set it to use require, but wanted to use assert you'd suite.assert().equal() as you mentioned before, so you'd by no means be stuck with one, or the other within that suite.

[boyan-soubachov]

Completely agreed, you would not be stuck with one. If we take a (relatively) practical example, the solutions would look as follows: -switching approach-

suite.UseRequire()
suite.Equal(...)
suite.UseAssert()
suite.Equal(...)
suite.Equal(...)

suite.UseRequire()
suite.Contains(...)

vs. -explicit approach-

suite.require.Equal(...)
suite.assert.Equal(...)
suite.assert.Equal(...)

suite.require.Contains(...)

In my opinion the latter is more concise and tends to read better as user does not have to scroll up and keep context of whether we're in require or assert 'mode'.

[Blquinn]

I wasn't thinking about having a method to change which mode you're in, but rather you'd just set it once when you initialize your suite.

type MySuite struct{
    suite.Suite
}

func TestSuite(t *testing.T) {
    suite.Run(t, &MySuite{
        Suite: suite.Suite{defaultAssertions: &assert.Assertions{}}
        // suite.Suite{defaultAssertions: &require.Assertions{}} // This would use the require assertions
       // not setting either would either panic on the nil interface, or we could have it default to one
    })
}

func (s *MySuite) TestSomething() {
    s.Equal(1, 1) // Would use assert because it is set above
    s.require().Equal(1, 1) // We could use either as normal
    s.assert().Equal(1, 1)
}

Wow that's a lot of the word suite :laughing:

[boyan-soubachov]

I see what you mean. I'm leaning towards dropping the implicit 'assert/require' altogether. That way there can be no state and implied behaviour. This would be stateless (i.e. every time declare suite.assert... or suite.require...) and clearer in its expression IMO

[boyan-soubachov]

@glesica , @mvdkleijn , thoughts on this?

In my opinion, we should drop the assert & require fields from the suite altogether and simplify it a bit?

[ghostsquad]

No don't drop the fields! I can see the benefit of s.Equal but some of us still want to be explicit.

[mvdkleijn]

I have read both issues and what I would like to do is:

• Drop the implicit behaviour of using asserts
• Keep both the assert and require fields
• Allow specifying the default behaviour at initialization of the Suite
• The default behaviour not being set should error out of the Suite

This would allow us to force the user to explicitly choose either assert or require and thus prevent any possible confusion.

It would also make using the Suite more convenient to use by allowing to choose to use an implicit behaviour by setting a default.

It would also still allow users that choose a default to override that default easily when needed.

In my opinion that would fit the needs of

• Testify, by having / promoting explicit behaviour
• Require only type users
• Assert only type users
• Choose one but occasionally override users
• Always explicit users

Hope that's clear and makes sense. Tell me if not, I'm writing from my phone :smile:

[Blquinn]

@mvdkleijn That is essentially my original proposal, other than the idea to drop the implicit behavior.

To be clear, you want to return an error when users use suite.Equal et al, correct? That would break everyone's existing tests until they somehow choose which one they want.

I think having a default behavior and having documentation showing how to do it is good enough, no? Plus no breaking change. It's not like this causes false positives in your tests, it just creates a lot of noise if you wanted the require behavior.

[mvdkleijn]

@Blquinn Your original idea proposes a RequireSuite which I don't agree with. However, I do like the idea of the user being able to explicitly choosing to use asserts vs requires.

However, in that same vein, I also like the idea of the Suite not having a default behaviour. The unfortunate truth is that loads of people don't read or gloss over documentation and this would be a simple thing to miss. Consequently it might cause confusion for some as they might expect require to be the default instead of assert, causing non-optimal tests.

Whether it is an error or a panic for example doesn't really matter much to me. (Though panic would probably be most appropriate) I was just toying with the idea of adding a helpful message for the user in cases like this. That would also promote the docs more.

For example, something like:

[PANIC] set a default assertion type for your Suite to use.

or even:

[PANIC] set a default assertion type for your Suite to use. See https://pkg.go.dev/github.com/stretchr/testify/suite?tab=doc#Suite

[Blquinn]

It would appear that I forgot what my initial proposal was :)

I do see your point about not reading documentation and I agree.

I was just trying to point out that we could do this in a way which is not a breaking change. We could even add it now as an incremental change, then add the panic later for a new major version. IE remove the default in the next version.

In any case, I think this feature would be most convenient if it was configurable globally, as well as per suite. So you could somehow set the global default for assert vs. require, but then override that default in each suite. Personally, I would want to be able to always use require, but then use assert for certain tests where I want to see all of the failures at once.

[mvdkleijn]

Actually, doing this in a two step incremental is fine by me... we could do most of the stuff for 1.7.0 and remove the default (i.e. make it an explicit choice) in the 2.0.0 milestone.

@boyan-soubachov What do you think about splitting this into two incremental adjustments? (would be nice-n-agile? :smile: )

We don't really set anything globally right now. You just import and start writing tests / suites. I'm not tempted to start doing that to be honest.

So...

For 1.7.0 (assuming @boyan-soubachov agrees)

• [ ] Keep both the assert and require fields
• [ ] Allow specifying the default behaviour at initialization of the Suite
• [ ] Not setting the default behaviour would fall back to using asserts as is the current implicit behaviour

For 2.0.0

• [ ] Drop the implicit behaviour of using asserts
• [ ] The default behaviour not being set should panic with a helpful message

[boyan-soubachov]

I personally have an aversion to state where it's not absolutely needed. Having to set a state before using the suite brings a few major downsides that would probably be quite common:

• Makes it more prone to human error. e.g. forgot to set require/assert => panic
• Having to switch between assert/require sections of the same test and the cognitive load required for that (https://github.com/stretchr/testify/issues/829#issuecomment-598416169)
• Tests not behaving as expected (e.g. not breaking on first error because we forgot that we're in an 'assert' rather than a 'require' suite).

This all, IMO, adds complexity where there needn't be.

If, however, the community does feel strongly about this, then I'm happy to commit to it

[Blquinn]

While I agree that needlessly adding state is a bad thing, I also don't want to have to type suite.require.Equal(1, 1) every time I do an assert. After all one of the best things about this library IMO is that it cuts away all the boiler-plate in my tests. That may not seem like much, but if you're a lunatic about testing, it adds up and you end up with carpal tunnel :smile:

Furthermore, this is more like configuration, than the devil's mutable state. User's won't be changing this during the run of a suite's tests.

I think this can be structured in a way to get the best of both worlds, but we may not have found that solution in this issue yet.

[ghostsquad]

I agree. Basically the ability to fail fast or slow is a feature of this library. You need some way to configure it.

If you entirely use one over the other, I think its likely very easy to make a helper function to reduce some of the boilerplate. The benefits are clear though, forcing you to be aware of the "default" behavior, since you set it yourself, while also allowing you to mix and match the behavior with explicit calls to require/assert.

[mvdkleijn]

Speaking from a personal standpoint, I don't really see the cognitive load issue here since the intended use would not really involve any switching. A user would opt to either make use of assert or require as the default and could occasionally choose to explicitly override that choice. As such I don't really see this as state but more as configuration. (I know... semantics :smile: )

Being (forced to be) explicit as a user is good in my opinion since it removes possible points of confusion and forces you to think about what you want, if only for two seconds.

Something like suite.RequireSuite would necessitate introducing an suite.AssertSuite as well for consistency's sake and leaving suite.Suite kinda ... dunno.

I quite like the idea of setting a default and being able to override it though.

I guess what is also possible:

1. Introduce RequireSuite that defaults to using require
2. Introduce AssertSuite for consistency
3. Change Suite to force user to choose and no longer default to using asserts
4. All of the above would allow the user to explicitly override the choice by doing suite.assert.Equal(...) or some such.

Though I'm not sure what I think of that myself. :rofl:

[Blquinn]

I think we decided that my idea to have separate suites RequireSuite + AssertSuite was not the prettiest solution. When I created my own it also broke IDE support for running individual test's in Goland. Not that that should stop anyone from making breaking changes if necessary. I mostly came up with that out of a desire to not introduce breaking changes. Since there is major version changed planned there's more flexibility.

I think #3 is the best option currently.

Could a 4th option be having multiple suite.Run variants, or having run take a config or something? Not saying that's prettier than the RequireSuite, just throwing it out there.

A builder pattern or functional composition of the suite could be used to configure the suite in a prettier way than setting the struct field. We could also expose less of the suite's internals that way.

suite.Run could take variadic configuration options (functional composition):

type Option func(*Suite)

var WithRequire = func(s *Suite) {
  // Set suit to use require
}

var WithAssert = func(s *Suite) {
  // Set suit to use require
}

// Could default to assert for now to not break api
func Run(suite *Suite, opts ...Option) {
 // apply options and run suite
}

Mainly pointing out that we have tons of options here... The functional composition one is actually not terrible, plus that's also not a breaking change.

[ghostsquad]

Frankly, it feels like the strongest opinion here (what this entire conversation is revolving around) is how do we keep suite.Equal(...) while behind the scenes changing whether or not that fails fast or slow.

Honestly, maybe it's more confusing that suite.Equal could either halt or not depending on some configuration. What about the return value?

if suite.Equal(...)

This only makes sense when set to assert mode.

why not just require the below syntax?

r := suite.Require

r.Equal(...)

Leverage IDE's built in ability for snippets if you don't want to type as much. https://code.visualstudio.com/docs/editor/userdefinedsnippets https://blog.jetbrains.com/go/2019/05/21/custom-live-templates-in-goland/

---

aside, functional options are an amazing pattern. Thank you @Blquinn for bringing that up.

[Blquinn]

@ghostsquad You can do r := suite.Require() already today.

This feature is really intended specifically to remove the need to litter your tests with r := suite.Require(). The need for snippets are an indicator of a badly designed API in my opinion.

[ghostsquad]

@Blquinn tests, by very design, are supposed to be explicit, and can thus be repetitive. Golang is optimized for reading the code, since you are likely to do that a lot more than you write it. So writing the code takes a bit longer, and ideally that results in a benefit that "the code does what it says on the page".

[Blquinn]

@ghostsquad You're not forced to do anything, you'll have both options available to you. Configuring the suite (at the top of the page of every test) is also explicit, it's just not as repetitive. Repetitive boilerplate makes code harder to read, not easier.

[mvdkleijn]

Let's try to prevent a flame war of sorts. :smile: All sides have presented valid arguments and they are clear. It is (mostly) a matter of taste at this point. I, for one, will let this all simmer a little in my tiny brain for the next week or so.

[boyan-soubachov]

Good arguments. Looking back on it, I think it would probably make sense to have some way of setting a suite's default testing behaviour.

As for how to do it: Would a functional parameter not make sense in this case? Or a receiver function? e.g. receiver:

mySuite := suite.Suite().RequireByDefault()

or functional parameter (similar to https://github.com/stretchr/testify/issues/829#issuecomment-668629018):

mySuite := suite.Suite(suite.RequireByDefault())

[Blquinn]

@boyan-soubachov I think they accomplish the same thing so it's really a matter of taste.

It seems to be more common to use the functional parameter, though I'm not sure why. Perhaps it's because chaining methods in go can be a little ugly where you have to put the period on the previous line to the method call. Most likely it's because people have seen builder patterns in java and therefore it is bad :laughing:

[orenl]

@Blquinn, if you choose (require/assert) at compile time then you can do the following (per struct, or as a base to other test structs):

type Suite struct {
    suite.Suite
    *require.Assertions
    ...
}

func TestWithRequire(t *testing.T) {
    suite.Run(t, &Suite{Assertions: require.New(t)})
}

func (tt *Suite) TestSample() {
  tt.Equal(1, 2)
  println("not reached")
}

If there was a suite.Assertions interface with all the methods (in each of require, assert), then we would be able to choose in runtime using something like this:

// toy imagined code assuming existence of `suite.Assertions` interface

type Suite struct {
    suite.Suite
    suite.Assertions
    ...
}

func TestWithEitherAssertOrRequire(t *testing.T) {
    var assertions suite.Assertions = assert.New(t)
    if youPreferRequire() {
        assertions = require.New(t)
    }
    suite.Run(t, &Suite{Assertions: assertions})
}

func (tt *Suite) TestSample() {
  tt.Equal(1, 2)
  println("reached only if youPreferRequire() == true")
}

(Of course, you can locally define type Assertions interface { ... } with all those methods, but it's just tedious, and really belongs in the "suite" package IMO).

As far as I can tell, adding such interface would be fully backward compatible as would not modify the suite.Struct in any way.

@boyan-soubachov, would you consider adding such suite.Assertions interface?