ErrorIs assertion; go1.13 feature

[nikandfor]

ErrorIs assertion to check errors as recommended to do in go1.13 release.

In addition to

assert.EqualError(t, err, "error text")

I've added support for new assertion

assert.ErrorIs(t, err, pkg.ErrBaseError)

to follow recommendations from errors package.

I've also added small comment on how to contribute to assertions because it took some time from me to find where to add code.

[nikandfor]

Oh, I see, we need to handle build tags somehow

[nikandfor]

I've updated _codegen. I didn't found simple solution so it changed significantly.
Now it checks build tags in each file and creates new files accordingly.

[boyan-soubachov]

Hi, thank you for your submission. We've been facing a similar situation recently with regards to code unsupported by older versions of Go.

We definitely want this feature but think it would be best if we put it on hold for now as we add more PRs to the version 2.0.0 release where we can cleanly and neatly drop support for older Go versions.

@glesica , as mentioned earlier. We should probably put this in the 'Next +1' milestone as we have a lot of merged content for a non-breaking change (i.e. 1.5.0). IMO 'Next' should be v1.5.0 with non-breaking changes and #872 and this PR should go into 'Next +1' (v2.0.0). Thoughts?

[nikandfor]

All the point of rewriting _codegen was ability to add 1.13 feature without breaking changes.
I've hidden ErrorIs under build tags so everything would compile and work on older go versions.
Since all tests are passed including go1.8 I think it could be merged into the next minor release.
Or have I missed something?

[glesica]

@boyan-soubachov yeah that sounds reasonable as long as we haven't already merged any breaking changes. I suspect we have, but I'm not positive.

[boyan-soubachov]

@nikandfor our concern is that we’re fragmenting the code base into different copies for different Go versions.

We had this issue recently (#872) and decided at the time that it’s probably better to release a breaking/major change than fragment the code base into different files for different versions.

If we feel that may not be the best way forward, I will support the endeavour. We should however have a discussion and give it some thought on what the best way of fragmenting the code base for different versions would be.

My personal, subjective opinion is that, given the broad scope the codebase covers, it would be better to use major versions rather than build flags since maintaining n copies of the same function would become unwieldy very quickly.

Just my $0.02. Would greatly appreciate yours and @glesica ‘s opinion

[nikandfor]

Do fragmenting the code means just dropping support for 1.12 and older versions in release 2.0.0?
Isn't it too rock? Or you want to maintain two different branches at the same time?

I used language version build tag go1.13, it is set by the compiler so you don't need to do anything and you are getting support for all the versions automatically.
All the functions are in the only one copy, so it's easy to support them.

Merging this feature will make it possible to just go get library on any go version, get the same codebase, compile the code totally identical and got almost the same result except ErrorIs (other new features could be added as easy as before) would only be available on go1.13+ versions.

I can't understand where is drawbacks in build tags. They work opaque, users will never notice them.

[glesica]

I'd personally rather not use build tags because they make the codebase more difficult to work with (this is just my opinion). But if other people want to do the work, then sure, go for it.

[boyan-soubachov]

Do fragmenting the code means just dropping support for 1.12 and older versions in release 2.0.0?

No, what I meant is that the maintainers will have different files of code for different versions of Go. An example in this PR is where there is now assertions.go, assertions_go1.13.go, assertions_forward.go, assertions_forward_go1.13.go, etc...

Over time we will start seeing assertions_go1.14, assertions_go1.13.go, assertions.go, etc. And we will have to move functionality between different 'go version files' depending on whether its compatible with new versions or not.

Isn't it too rock? Or you want to maintain two different branches at the same time?

I used language version build tag go1.13, it is set by the compiler so you don't need to do anything and you are getting support for all the versions automatically.
All the functions are in the only one copy, so it's easy to support them.

Merging this feature will make it possible to just go get library on any go version, get the same codebase, compile the code totally identical and got almost the same result except ErrorIs (other new features could be added as easy as before) would only be available on go1.13+ versions.

I can't understand where is drawbacks in build tags. They work opaque, users will never notice them.

Our utmost concern is the users indeed, but we also need to consider developer efficiency. If we use versioning rather than build tags, we can have as is the Go recommended way for major versions. Example: Any breaking changes we've saved for the next major will then be available in github.com/stretchr/testify/v2. It will effectively be a separate package which will have the major changes applied in it and users can upgrade at will.

v1 will be retired from maintenance and any new releases will happen on the current major version only. Please let me know if this doesn't clarify what I meant.

P.S. Where I get my information on recommend versioning in Go https://blog.golang.org/versioning-proposal

[boyan-soubachov]

Is this not redundant to line 335? Would running go generate ./... not have the same effect?

[nikandfor]

335 says how to generate code, but doesn't say, where to add new one by hands.

[nikandfor]

Yeah, I wrote about go generate second time.

[boyan-soubachov]

What problem are we solving by moving all the functions to be receivers on Context?

[nikandfor]

Getting access to more data. To assign build tags to a function in analyzeCode I needed *token.FileSet (that wasn't in returning arguments) and a map to save file to tags relations.

Adding these new return arguments meant to have 4 of them. I decided it's time to store them all in a receiver struct.

[nikandfor]

I've already done it. _codegen will support all the new build tags without additional cost.

If some warranty case happen with tags later, mention me and I'll fix them. :)

[boyan-soubachov]

I've already done it. _codegen will support all the new build tags without additional cost.

If some warranty case happen with tags later, mention me and I'll fix them. :)

But _codegen does not take care of assertions.go if I'm not mistaken?

Would we not have to manually keep assertions.go, assertions_go1.13.go, assertions_go1.14.go, assertions_go1.15.go ... etc. in sync?

[glesica]

Another possibility would be to just not merge changes that aren't backward compatible and make 2.0 support 1.12.

[boyan-soubachov]

@glesica , I'm happy for you to make a call on this one :)

My personal, views is that we should look at sticking with Go's de facto major versioning system (https://blog.golang.org/versioning-proposal) rather than using build tags.

Reason for:

• Only one set of code/source files to maintain
• Predictable/follows convention
• Allows us to cut new releases in exactly the same way we have been doing previously. i.e. relatively simple to understand

Reasons against:

• Users will have to import github.com/stretchr/testify/v2 if they want to migrate to the new major version

[boyan-soubachov]

Another possibility would be to just not merge changes that aren't backward compatible and make 2.0 support 1.12.

True, but at some point we will have to bite the proverbial bullet and actually release some backwards-incompatible code as the community wants it. :)

[nikandfor]

Would we not have to manually keep assertions.go, assertions_go1.13.go, assertions_go1.14.go, assertions_go1.15.go ... etc. in sync?

What do you mean in sync? They are all independant.

[nikandfor]

• Only one set of code/source files to maintain

There would be only one set of functions to maintain. File with go version tag adds one new function but not having all of them as another copy.

[nikandfor]

• Predictable/follows convention

build tags follow convention either. And they are predictable as for me

[nikandfor]

Let me explain how it works with tags.

// assertions.go
// no build tags here

// all the functions that were here unchanged

// assertions_go1.13.go
// +build go1.13

// one new function
func ErrorIs(/*...*/) { 
// ...
}

_codegen parses assert package, takes all the functions, select which follows format (first arg is testingT and so on), finds build tags for each function and generates other forms of assertions as before, writing functions in files under the same build tag they were originally were.

As result would be generated new files:

// assertions_format.go
// no build tags here
// generated just as before

// all the same functions as before

// assertions_format_go1.13.go
// +build go1.13
// generated automatically by the same rules

// one new function
func ErrorIsf(/*...*/) {
// ...
}

You won't get any new headache to support.

[boyan-soubachov]

What is the purpose of changing everything to reference your fork?

[nikandfor]

I decided you won't accept this pull request so I create a fork with build tags support.
Which actually wasn't a good idea because I have my own projects and It's not enough time for all of it.
I didn't found a way to change branch here so I can reopen new pull request from the same state as before but with different branch name. Should I?

[nikandfor]

I've also renamed _codegen to .codegen and it prevented both repos imports to blend.
For example it allows to run go test ./... with no go.mod changed.
Should I include this change?

[glesica]

Please remove all changes that aren't relevant to the feature you're adding, then we can review that more easily. If you want to submit additional PRs for the other changes, that's fine, we can review those separately.

[dolmen]

The _codegen refactor in this PR is interesting. It would be good to have it in a separate PR.

ErrorIs has since been implemented without build tags.