mock: isolate objx-dependent code in their own source files

[dolmen]

Summary

In order to ease maintenance of downstream forks of Testify that would remove dependency on github.com/stretchr/objx we move all uses of that module in isolated source files. A fork without that dependency could just remove those files.
See #1752 (comment)

The use of objx is quite contained: it is only used in Mock.TestData(). Note that we can't just remove that method or change the return value because that would be a breaking change.

Changes

• Move Mock.TestData() in a new file mock/mock_objx.go
• Move tests of TestData in a new file mock/mock_objx_test.go

Motivation

See #1752 (comment)

With this change it will also be possible to add a //go:build !testify_mock_no_objx to mock/mock_objx.go and mock/mock_objx_test.go to allow downstream users to use testify/mock without the objx dependency.

Related issues

• Do we need objx dependency? #1752

[brackendawson]

In my opinion I don't think this is a worthwhile change, removing my review without blocking.

This change also does the opposite of what it intends: If a fork of testify has removed objx then this change will be a merge conflict when they pull from upstream. If we touch nothing in the objx referenced code then it will not be a merge conflict for them.

[dolmen]

With this change it will be possible to add a //go:build !testify_mock_no_objx to mock/mock_objx.go and mock/mock_objx_test.go.

[brackendawson]

This PR achieves the opposite of its intention. The stated aim is to make it easier for forks of testify to remove the objx dependency by moving code containing references to objx to new files.

Every time we touch the code referencing objx we cause a merge conflict for such forks and that includes this change. We should not make any change to make it easier for forks unless we also have to make another change to lines which reference objx.

With this change it will be possible to add a //go:build !testify_mock_no_objx to mock/mock_objx.go and mock/mock_objx_test.go.

This will still not allow such a fork to remove objx from go.mod.

Please reference a request from the maintainer of a fork for this functionality.

[dolmen]

This will still not allow such a fork to remove objx from go.mod.

An automated fork that tracks our master would be easy to do. Just a single patch that would be rebased. I think that resolving conflicts which involve just removing 2 files and go mod tidy would be easy to automate.

Please reference a request from the maintainer of a fork for this functionality.

Of course no such fork exists yet.

But I think that:

1. it would be easy to create
2. it would be easy to automate once this change is accepted. But I need this change to be accepted to demonstrate it.
3. we could provide that fork ourself on a branch in this repo master-no-objx automatically updated by CI

[ccoVeille]

I don't think it's worth trying this

[dolmen]

@brackendawson You wrote

Please reference a request from the maintainer of a fork for this functionality.

Are you aware of a such fork already existing?

[brackendawson]

I'm not aware of any. I think this feature should not be done unless it delivers good value to a customer who exists.

[ccoVeille]

This will still not allow such a fork to remove objx from go.mod.

An automated fork that tracks our master would be easy to do. Just a single patch that would be rebased. I think that resolving conflicts which involve just removing 2 files and go mod tidy would be easy to automate.

Please reference a request from the maintainer of a fork for this functionality.

Of course no such fork exists yet.

But I think that:

1. it would be easy to create
2. it would be easy to automate once this change is accepted. But I need this change to be accepted to demonstrate it.
3. we could provide that fork ourself on a branch in this repo master-no-objx automatically updated by CI

I have read everything again (this PR but also PR that were done in objx and testify itself), and I changed my mind. I feel like we should try.

At least we should add a deprecation warning as soon as possible

[fredbi]

@ccoVeille @brackendawson

I am discovering this piece of work.

From recent experiments at managing dependencies, I can tell for a fact that using build tags to guard dependencies does not achieve anything: consumers of the lib still get the dependencies in their go.mod.

the only viable way i found to make dependencies optional (like stuff used only in the specific use-casz of mocks) is to make an independent module.

this is doable but managing a mono repo with multiple modules requires a bit of extra attention.

i could transform the idea in that direction if you like it

[brackendawson]

@fredbi in the case where assert.YAMLEq has to work without an initialisation by the users; does a separate module even remove the dependency from the primary module? We would have to import it, no?

[fredbi]

Yes you would have to import it explicitly

Initialization may be explicit (e.g call « module.RegisterSomeFeature()”) or may be implicit (eg blank import calls init).

this would work for the yaml part but also for any “niche” dependency caused by some specific part of the framework (e.g mocks, testsuites and other added chrome - like colorized output and what not)

If you’re interested i can showcase the concept with a recent successful deep transformation of a similar “large bandwidth” library that i’ve sliced in a dozen highly focused modules.

[ccoVeille]

I like your ideas for YAMLEq.

Here objx is very limited I think. It's about having a struct if I remembered well.

Can't we simply use a replace directive in our project, to force to use a local lib?

Would it work? Coukd it help to solve the dependency issue?

Yes, I agree with you the build tags things looks like an idea that brought nothing except a dead end

[fredbi]

No replace directives only act locally not for the consumers of your package

[fredbi]

@ccoVeille since you know the place, take a look at github.com/go-openapi/swag. The replace directives exist only for local development and to be able to cut releases (since at this very moment you put tags in the go.mod’s that don’t exist yet)

[fredbi]

Also notice that go.work (go1.18+) is by no means essential. It just provides a slightly improved developer experience (as it was designed for). Replace directives are essential for the multi-module with inter-dependent modules, but not resolved by the consumers of your modules

[fredbi]

The main issue in testify is to support optional dependency without bringing a breaking change. And that part is hard.

I don’t it is reasonable to expect absolutely no breaking change (apart by reimplementing these deps with the std library only). There the testify linter could help, with an autofix to add the desired import wherever we call something that needs an extra import. Could be YAMLEq, but also the stuff mentioned above related to the mock.

on a given code base, only a tiny fraction would actually use YAMLEq so i’d expect a limited impact

[ccoVeille]

No replace directives only act locally not for the consumers of your package

Thanks for the confirmation. I assumed it was something like that.

@ccoVeille since you know the place, take a look at github.com/go-openapi/swag. The replace directives exist only for local development and to be able to cut releases (since at this very moment you put tags in the go.mod’s that don’t exist yet)

👍

[ccoVeille]

The main issue in testify is to support optional dependency without bringing a breaking change. And that part is hard.

I don’t it is reasonable to expect absolutely no breaking change (apart by reimplementing these deps with the std library only).

Yes, it's an option I thought about.

Import the minimal code needed to check a YAML is equivalent to another, and remove the dependency. Here it's possible because we are not exporting yaml type out of testify.

objx is different as it exposes a type from the dependency.

YAMLEq would require to copy a lot of thing, but it could be a better option as the scope is very limited.

There the testify linter could help, with an autofix to add the desired import wherever we call something that needs an extra import. Could be YAMLEq, but also the stuff mentioned above related to the mock.

on a given code base, only a tiny fraction would actually use YAMLEq so i’d expect a limited impact

This would be a breaking change

[fredbi]

An intermediate position could be to indulge into “blessed” dependencies only: objx is part of the stretchr org (or could be a module inside testify) and that’s fine by me. When consumers of my packages see in its go.mod all the imports “stretchr/testify/xxx” they instantly get that this is test dependencies and moves on.

my auto-merging bots can recognize those as blessed just as well.

but the yaml stuff, the spew lib and the go-difflib we should IMHO isolate somehow

[fredbi]

Here is yet another possible strategy (i am planning to apply that one on another lib, to wit girhub.com/go-openapi/runtime, another large-bandwidth stuff we have).

There i want to cut the dependency to open telemetry. But i can't do that without a breaking change.

so what i am planning is that i’ll freeze the dependency, publish the deprecation and encourage users to use the newly published module which is maintained with regular updates.

this is a bit similar to your idea of copying go-yaml once to ensure it works, but would relieve the maintainers from the burden of further maintaining that copy.