assert: make YAML dependency pluggable via build tags

dolmen commented 3 months ago

Summary

Make the YAML module dependency required for assert.YAMLEq (and family) pluggable.

Changes

Add package github.com/stretchr/testify/assert/yaml that wraps the dependency. It exposes just func Unmarshal([]byte, interface{}) error (the only function imported from gopkg.in/yaml.v3 for assert).
Add 3 implementations of that package which are activated by build tags. The default implementation is the existing behavior and just wraps gopkg.in/yaml.v3.Unmarshal.

Motivation

The dependency on gopkg.in/yaml.v3 for YAML deserialization (used by assert.YAMLEq) is causing painful maintenance work. Both for this project, and for downstream projects (end users). Unfortunately we can't just drop assert.YAMLEq until v2.

However this change allows to mitigate the issue by giving freedom to users to avoid the link constraints. This allows:

avoid linking constraints of the license of gopkg.in/yaml.v3: see #1120
should avoid future shower of irrelevant reports of vulnerabilities related to bugs in gopkg.in/yaml.v3: like previous #1192, #1193, #1194, #1280, #1241, #1292, #1532 (I wrote irrelevant because selecting a particular version of a module is the final responsibility of users in downstream projects who assemble a program: downstream projects have all the infrastructure in the Go toolchain to handle such issues without harassing upstream projects' maintainers)

The github.com/stretchr/testify/assert/yaml implementation can be selected using build tags:

testify_yaml_default (default): gopkg.in/yaml.v3 is used, like before
testify_yaml_fail: YAML deserialization is not implemented and always fails. So assert.YAMLEq always fails. This is useful if the test suite package doesn't use assert.YAMLEq (very common case).
testify_yaml_custom: the (new) github.com/stretchr/testify/assert/yaml package exposes an Unmarshal variable of type func([]byte, any) error (same as gopkg.in/yaml.v3.Unmarshal) that allows to plug any alternate implementation. For example github.com/goccy/go-yaml.Unmarshal.

Usage:

go test -tags testify_yaml_fail
go test -tags testify_yaml_custom

To install an alternate implementation with testify_yaml_custom (as requested in #1120):

//go:build testify_yaml_custom

package my_pkg_test

import (
    goyaml "github.com/goccy/go-yaml"
    "github.com/stretchr/testify/assert/yaml"
)

func init() {
    yaml.Unmarshal = goyaml.Unmarshal
}

Related issues

1120

dolmen commented 3 months ago

Cc: @Al2Klimov as you proposed #1120. I think this should fit you use case.

brackendawson commented 3 months ago

I am hesitant to use this approach. Are there any unforseen consequences? This will not remove yaml from the go.mod or that of any of module importing assert.

Yes, you can show that you don't actually use yaml in your build, but you can already show that you don't compile it in as you only use it in your test.

If the original reporter does actually have utility for this then I might be more swayed.

dolmen commented 3 months ago

Are there any unforseen consequences?

None.

This will not remove yaml from the go.mod or that of any of module importing assert.

In fact this doesn't. But there is nothing we can do about that as long as we keep the assert.YAML* functions.

Yes, you can show that you don't actually use yaml in your build, but you can already show that you don't compile it in as you only use it in your test.

In #1120 I expect the requester didn't want even to have gopkg.in/yaml.v3 linked when running tests as that would raise a licencing issue. The -tags testify_yaml_fail would enforce that. The user could use a //go:build testify_yaml_fail for all of its testsuite sources to ensure that his testsuite never runs with gopkg.in/yaml.v3 linked.

dolmen commented 3 months ago

Note that I have also serious concerns about the state of the maintenance of gopkg.in/yaml.v3. Here are 2 PR that I filled myself and that haven't yet got reviews:

go-yaml/yaml#1027
go-yaml/yaml#1028

dolmen commented 1 month ago

Cc: @jasdel In project github.com/jmespath/go-jmespath you vendored Testify at v1.5.1 because of the upgrade of go-yaml from v2 to v3. As the go-jmespath testsuite doesn't use YAML function at all, I think that this change could help you to drop that fork and just use the maintained version of Testify. What do you think about it? Could you review the changes?

jasdel commented 1 month ago

@dolmen thanks for the ping. I'm not a maintainer of that project anymore. But with that said, splitting the yaml as a non-required dependency would address the issue I ran into when originally filing the issue.

In the case of go-jmespath I'd need to set the build flag testify_yaml_fail to remove the runtime dep, correct? That makes sense for an app.

The only call out about this is that libraries or another app that depend on go-jmespath they will still have the yaml dep in its chain, as indirect dependency. Though, go-jmespath probably should be updated to use the latest version of testify. If the repo is still maintained, @jamesls might be interested in making that update.

andig commented 1 week ago

Note that I have also serious concerns about the state of the maintenance of gopkg.in/yaml.v3. Here are 2 PR that I filled myself and that haven't yet got reviews:

@dolmen OT, but I've opened https://github.com/go-yaml/yaml/issues/1034

stretchr / testify