[Fix] Generate mock build constraints #693

mcdoker18 · 2023-08-16T10:59:49Z

Supported only the new format of build tags
https://pkg.go.dev/cmd/go#hdr-Build_constraints

Fixes #691

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Fixes # (issue)

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Version of Golang used when building/testing:

How Has This Been Tested?

Unit and e2e tests are provided.

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

mcdoker18 · 2023-08-16T11:10:16Z

@LandonTClipp

There is a little problem with the tag field in config. It is already being used with parser. By the way it's not documented behavior. And the worst thing is, the format differs for the parser and file.
For example:

parser - custom1,custom2
file - custom1 && custom2

LandonTClipp · 2023-08-16T15:44:07Z

Yikes, this looks like historical cruft that we need to address. Thanks for the PR. I'll give my thoughts in the coming days.

codecov · 2023-08-23T17:36:34Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b9df18e) 75.17584% compared to head (58b0c4c) 75.26793%.
Report is 5 commits behind head on master.

Additional details and impacted files

@@                 Coverage Diff                 @@
##              master        #693         +/-   ##
===================================================
+ Coverage   75.17584%   75.26793%   +0.09209%     
===================================================
  Files             10          10                 
  Lines           2417        2426          +9     
===================================================
+ Hits            1817        1826          +9     
  Misses           461         461                 
  Partials         139         139

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

LandonTClipp · 2023-09-08T02:16:27Z

I haven't forgotten about this PR, it's on my radar, just been busy with other priorities :)

LandonTClipp · 2023-09-21T19:08:09Z

mocks/github.com/vektra/mockery/v2/pkg/fixtures/buildtag/comment/IfaceWithBuildTagInComment.go

@@ -0,0 +1,87 @@
+//go:build custom && (!windows || !linux || !darwin || !freebsd)
+
+// Code generated by mockery. DO NOT EDIT.


We need to double check if this DO NOT EDIT comment being placed below the first line still meets the specification: golang/go#41196

I believe this doesn't violate the spec: https://pkg.go.dev/cmd/go#hdr-Generate_Go_files_by_processing_source

This line must appear before the first non-comment, non-blank text in the file.

Thanks for pointing it out, I will fix it.

By the way boilerplate text is generated before // Code generated .... Is it correct?

LandonTClipp · 2023-09-21T19:22:49Z

cmd/mockery.go

@@ -381,6 +381,7 @@ func (r *RootApp) Run() error {
 		InPackage:            r.Config.InPackage,
 		KeepTree:             r.Config.KeepTree,
 		Note:                 r.Config.Note,
+		BuildTags:            r.Config.BuildTags,


I have (at least) two questions.

So, I'm confused about this line. Is this PR attempting to fix this behavior for both packages and non-packages configurations? Or are you intending to fix it for one or the other?

My other question, is this okay for us to be passing the build tags into packages.Load, and to inject those same build tags as a comment into the mock file? My intuition says yes, but I fear there might be a reason why someone would want different tags for the package parsing, and another for the mock file. But I just can't foresee what would be desirable here.

What is non-packages configuration? Do you mean default top-level configuration?

No, I think that is wrong. Because they have different syntax:

for generated mocks - https://pkg.go.dev/cmd/go#hdr-Build_constraints

for packages.Load - a comma-separated list

Now tag config field is used for loading packages but documentation says for generated mocks.

Maybe we need two different config options for generated mocks and package loading?

What is non-packages configuration? Do you mean default top-level configuration?

I mean usage of the legacy configuration semantics, i.e. things that do not use this.

Now tag config field is used for loading packages but documentation says for generated mocks. Maybe we need two different config options for generated mocks and package loading?

Yeah, I am thinking this is what we ultimately want to do. Why don't we go ahead and add two new config options:

PackagesLoadTags MockBuildTags

I think that should make it clear enough what is intended?

To make this more clear, as I had to remind myself too, the current BuildTags field gets added to the configuration of the pkg.Parser object. This ultimately makes its way to packages.Load here. So BuildTags is specifically meant for only packages.Load.

What we want, in addition to what we already have, is mock-build-tags which is what will ultimately get added as the //go:build directive. So this PR is incorrect in the sense that it's sending the BuildTags to the //go:build string, when it should only go to the packages.Load invocation.

Sounds good! I intend to add these fields only for the new configuration.

But what is about the current tags field? Should I leave it in the source code to be backward compatible and remove from the documentation?

I think we should leave it as-is. It's kind of poorly named, but I will make a note of it in the docs clearly explaining what it's used for. In v3 we can probably change the name to make it more obvious.

I added the new option and changed derscription of the tags option.

LandonTClipp · 2023-09-21T19:23:21Z

@mcdoker18 I have a few questions, lmk your thoughts

LandonTClipp · 2023-12-19T15:31:43Z

That would be great if you could push this further! I’ll be sure to get it merged as soon as my questions are addressed. Regards, -- *Landon Clipp*

…

On Tue, Dec 19, 2023 at 7:34 AM Vincent Gschwend ***@***.***> wrote: If this is stale I would be happy to fork and resubmit - we're currently testing mockery at Docker for some project and the tags are blocking us @LandonTClipp <https://github.com/LandonTClipp> @mcdoker18 <https://github.com/mcdoker18> :) — Reply to this email directly, view it on GitHub <#693 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACVWMAJJAI6543OJ5U2GLF3YKGJU5AVCNFSM6AAAAAA3SLFNSSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNRSG43DSNBWGY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

wobondar · 2024-01-03T10:58:10Z

Hi @LandonTClipp @mcdoker18

is there any chance of getting this merged anytime soon?

In order to overcome the issue, I've been forced to add the following:

mocks-build-tag:
	@echo "Adding mocks build tag to mock files"
	@if [ "$$(uname)" = "Darwin" ]; then \
		find ./mocks/ -name '*_mock.go' -exec gsed -i '/^package/ i //go:build mocks' {} +; \
	else \
		find ./mocks/ -name '*_mock.go' -exec sed -i '/^package/ i //go:build mocks' {} +; \
	fi

Much appreciated,
Andrew.

LandonTClipp · 2024-01-08T18:55:46Z

Hi @wobondar . I'm happy to merge this but I need the discussions to be resolved. I'm also happy to take a different PR as this one seems to be stale.

I might end up doing this myself at some point as multiple people have requested this.

mcdoker18 · 2024-01-09T21:34:36Z

@LandonTClipp I am very sorry. Somehow I skipped notifications from this PR :(

LandonTClipp · 2024-01-09T22:07:30Z

@LandonTClipp I am very sorry. Somehow I skipped notifications from this PR :(

No worries, thanks for checking back up! If you don't have the time to finish this out that's okay, just let me know. I appreciate any volunteer effort I can get, so I won't complain :D

Supported only the new format of build tags https://pkg.go.dev/cmd/go#hdr-Build_constraints Fixes vektra#691

mcdoker18 · 2024-01-10T16:17:33Z

@LandonTClipp I noticed couple of minor issues:

we can't use the option tags in the package configs. Because mockery takes only top level tags and passes it to he package loader.
Now the tags option is space-separated list, but in all other tools, like go build or go test, it is comma-separated list.

LandonTClipp

This is 99% of the way there, just a couple questions. Thanks again for helping out, I really do appreciate it.

LandonTClipp · 2024-01-10T17:38:34Z

docs/configuration.md

@@ -78,7 +78,8 @@ Parameter Descriptions
 | [`packages`](features.md#packages-configuration)       | :fontawesome-solid-x:     | `#!yaml null`                         | A dictionary containing configuration describing the packages and interfaces to generate mocks for.                                                                                                                                                  |
 | `print`                                                | :fontawesome-solid-x:     | `#!yaml false`                        | Use `print: True` to have the resulting code printed out instead of written to disk.                                                                                                                                                                 |
 | [`recursive`](features.md#recursive-package-discovery) | :fontawesome-solid-x:     | `#!yaml false`                        | When set to `true` on a particular package, mockery will recursively search for all sub-packages and inject those packages into the config map.                                                                                                      |
-| `tags`                                                 | :fontawesome-solid-x:     | `#!yaml ""`                           | Set the build tags of the generated mocks.                                                                                                                                                                                                           |
+| `tags`                                                 | :fontawesome-solid-x:     | `#!yaml ""`                           | A space-separated list of additional build tags to load packages.                                                                                                                                                                                                                |
+| `mock-build-tags`                                       | :fontawesome-solid-x:     | `#!yaml ""`                          | Set the build tags of the generated mocks. Read more about the [format](https://pkg.go.dev/cmd/go#hdr-Build_constraints).                                                                                                                                                        |


Can you place this alphabetically?

LandonTClipp · 2024-01-10T17:38:45Z

.mockery.yaml

@@ -4,7 +4,14 @@ with-expecter: True
 mockname: "{{.InterfaceName}}"
 filename: "{{.MockName}}.go"
 outpkg: mocks
+tags: "custom2"


What's the purpose of adding this?

I check that passing of build tags to package loader works in e2e tests. The IfaceWithCustomBuildTagInComment mock will not be generated without this line.

You can check by commenting this line and exec task test.e2e.

Ah got it! Makes sense.

LandonTClipp · 2024-01-10T17:40:58Z

we can't use the option tags in the package configs. Because mockery takes only top level tags and passes it to he package loader.

Ah right. That's not exactly intended but I don't see a way around that for the reason you mentioned.

Now the tags option is space-separated list, but in all other tools, like go build or go test, it is comma-separated list.

Actually, this is interesting. go help build shows this tidbit:

        -tags tag,list
                a comma-separated list of additional build tags to consider satisfied
                during the build. For more information about build tags, see
                'go help buildconstraint'. (Earlier versions of Go used a
                space-separated list, and that form is deprecated but still recognized.)

We can leave it for now considering it's still recognized.

LandonTClipp

Looks great, approved. I'll merge this into master and create a new build.

Since this is technically adding new functionality, I'll bump it as a minor revision.

LandonTClipp reviewed Sep 21, 2023

View reviewed changes

Generate mock build constraints

1c18f44

Supported only the new format of build tags https://pkg.go.dev/cmd/go#hdr-Build_constraints Fixes vektra#691

mcdoker18 force-pushed the fix/691 branch from b387c05 to 1c18f44 Compare January 10, 2024 16:11

LandonTClipp requested changes Jan 10, 2024

View reviewed changes

code review fixes

58b0c4c

LandonTClipp approved these changes Jan 12, 2024

View reviewed changes

LandonTClipp merged commit 2502f52 into vektra:master Jan 12, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix] Generate mock build constraints #693

[Fix] Generate mock build constraints #693

mcdoker18 commented Aug 16, 2023 •

edited

Loading

mcdoker18 commented Aug 16, 2023

LandonTClipp commented Aug 16, 2023

codecov bot commented Aug 23, 2023 •

edited

Loading

LandonTClipp commented Sep 8, 2023

LandonTClipp Sep 21, 2023

LandonTClipp Sep 21, 2023 •

edited

Loading

mcdoker18 Jan 9, 2024

mcdoker18 Jan 10, 2024

mcdoker18 Jan 10, 2024

LandonTClipp Sep 21, 2023

mcdoker18 Jan 9, 2024

LandonTClipp Jan 9, 2024

LandonTClipp Jan 9, 2024 •

edited

Loading

mcdoker18 Jan 9, 2024

LandonTClipp Jan 9, 2024

mcdoker18 Jan 10, 2024

LandonTClipp commented Sep 21, 2023

LandonTClipp commented Dec 19, 2023 via email

wobondar commented Jan 3, 2024

LandonTClipp commented Jan 8, 2024

mcdoker18 commented Jan 9, 2024

LandonTClipp commented Jan 9, 2024

mcdoker18 commented Jan 10, 2024

LandonTClipp left a comment

LandonTClipp Jan 10, 2024

mcdoker18 Jan 10, 2024

LandonTClipp Jan 10, 2024

mcdoker18 Jan 10, 2024

LandonTClipp Jan 10, 2024

LandonTClipp commented Jan 10, 2024

LandonTClipp left a comment

		@@ -0,0 +1,87 @@
		//go:build custom && (!windows \|\| !linux \|\| !darwin \|\| !freebsd)

		// Code generated by mockery. DO NOT EDIT.

[Fix] Generate mock build constraints #693

[Fix] Generate mock build constraints #693

Conversation

mcdoker18 commented Aug 16, 2023 • edited Loading

Description

Type of change

Version of Golang used when building/testing:

How Has This Been Tested?

Checklist

mcdoker18 commented Aug 16, 2023

LandonTClipp commented Aug 16, 2023

codecov bot commented Aug 23, 2023 • edited Loading

Codecov Report

LandonTClipp commented Sep 8, 2023

Choose a reason for hiding this comment

LandonTClipp Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LandonTClipp Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LandonTClipp commented Sep 21, 2023

LandonTClipp commented Dec 19, 2023 via email

wobondar commented Jan 3, 2024

LandonTClipp commented Jan 8, 2024

mcdoker18 commented Jan 9, 2024

LandonTClipp commented Jan 9, 2024

mcdoker18 commented Jan 10, 2024

LandonTClipp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LandonTClipp commented Jan 10, 2024

LandonTClipp left a comment

Choose a reason for hiding this comment

mcdoker18 commented Aug 16, 2023 •

edited

Loading

codecov bot commented Aug 23, 2023 •

edited

Loading

LandonTClipp Sep 21, 2023 •

edited

Loading

LandonTClipp Jan 9, 2024 •

edited

Loading