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

assert: make YAML dependency pluggable via build tags #1579

Merged
merged 1 commit into from
May 23, 2024

Conversation

dolmen
Copy link
Collaborator

@dolmen dolmen commented Mar 19, 2024

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:

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

@dolmen dolmen added enhancement dependencies Pull requests that update a dependency file internal/cleanup Internal change to ease maintenance without external impact YAML About YAML and dependency labels Mar 19, 2024
@dolmen dolmen self-assigned this Mar 19, 2024
@dolmen
Copy link
Collaborator Author

dolmen commented Mar 19, 2024

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

Make the YAML dependency required for {assert,require}.YAMLEq{,f} pluggable.

The 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 github.com/stretchr/testify/assert/yaml
  package exposes an Unmarshal variable of type func([]byte, any) error
  (same as gopkg.in/yaml.v3) that allows to plug any alternate
  implementation. For example github.com/goccy/go-yaml.Unmarshal.
  This allows to avoid the link constraints of the license of
  gopkg.in/yaml.v3 (see PR #1120).

Usage: go test -tags testify_yaml_fail

To install the alternate implementation with testify_yaml_custom:

	//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
	}
@brackendawson
Copy link
Collaborator

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
Copy link
Collaborator Author

dolmen commented Mar 31, 2024

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
Copy link
Collaborator Author

dolmen commented Mar 31, 2024

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
Copy link
Collaborator Author

dolmen commented May 6, 2024

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
Copy link

jasdel commented May 22, 2024

@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.

@MovieStoreGuy MovieStoreGuy merged commit 6b275ad into master May 23, 2024
14 checks passed
@andig
Copy link

andig commented Jun 22, 2024

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 go-yaml/yaml#1034

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement internal/cleanup Internal change to ease maintenance without external impact YAML About YAML and dependency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants