add a test for default-gen output #55

caesarxuchao · 2017-05-12T22:54:27Z

k8s-reviewable · 2017-05-12T22:54:32Z

This change is

caesarxuchao · 2017-05-12T23:31:03Z

@mbohlool could you help review it? Thanks.

mbohlool · 2017-05-15T23:21:08Z

examples/defaulter-gen/Makefile

+	fi
+	@go build -o /tmp/$(TOOL)
+	@PKGS=$$(go list ./output_tests/...  | paste -sd' '); \
+	/tmp/$(TOOL) --logtostderr --v=6 -i $$(echo $$PKGS | sed 's/ /,/g') -O zz_generated


so the paste command above is adding space between package names and sed here convert those spaces to comma, right? why not just use paste -sd','?

I copy-pasted it from https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/authorization/v1/types.go#L27. I can update both if you think it's necessary.

The link is referring to the wrong file? Anyway I think you can just fix it here if you like. It is not necessary though.

Sorry, https://github.com/kubernetes/gengo/blob/master/examples/deepcopy-gen/Makefile#L9

mbohlool · 2017-05-15T23:22:38Z

examples/defaulter-gen/output_tests/wholepkg/vendor/k8s.io/apimachinery/pkg/runtime/mock.go

@@ -0,0 +1,22 @@
+/*


Why do you want the generated file to be buildable? is the test only making sure that the output is what we expecting?

The travis builds the entire repo.

I can try to change .travis but that's more nasty IMHO :)

In that case, I suggest you rename the expected file zz_generated.go to zz_generated.expected, add the name zz_generated.go to git_ignore of that folder, and do a diff zz_generated.expected zz_generated.go instead of git diff ....

caesarxuchao · 2017-05-15T23:56:17Z

@mbohlool updated, ptal. Thanks!

mbohlool · 2017-05-15T23:58:20Z

LGTM

mbohlool · 2017-05-16T00:00:28Z

But it look like travis is still compiling it :( maybe remove the file after you checked the diff?

thockin · 2017-05-17T05:30:11Z

examples/defaulter-gen/Makefile

+	fi
+	@go build -o /tmp/$(TOOL)
+	/tmp/$(TOOL) --logtostderr --v=6 -i ./output_tests/wholepkg -O zz_generated
+	@if ! diff -q ./output_tests/wholepkg/zz_generated.expected ./output_tests/wholepkg/zz_generated.go; then \


why is this different than https://github.com/kubernetes/gengo/blob/master/examples/deepcopy-gen/Makefile#L11 ?

The generated code depends on apimachinery so it does not compile and fails travis. So i changed the suffix to .expected.

Why does this exist? The library has a --verify-only option specifically so that we would not write things like this any more.

We can change the default-gen Makefile to use --verify-only in a follow-up PR.

For this PR, because the generated code does not compile and breaks travis, i need to change the suffix of the output file to .expected, so the --verify-only approach doesn't work.

To use --verify-only, we have two options:

vendor a mock apimachinery folder in the test package to make the code compile.

hack the .travis to ignore this package.

@lavalamp please let me know how you want me to proceed. Thanks.

blech wrt having external deps. OK. Can you comment that?

The point of checking it in is so that a human can look at a series of trivial output tests, and comprehend the output. --verify-only doesn't give me that?

thockin · 2017-05-17T05:30:47Z

examples/defaulter-gen/output_tests/wholepkg/a.go

+}
+
+// No defaulter will be generated
+type Stuct_No_Field_Has_Manual_Defaulter struct {


s/Stuct/Struct/g

lavalamp · 2017-05-18T05:22:46Z

--verify-only tells you that the checked-in thing is still indeed what the generator would output.

…

On Wed, May 17, 2017 at 10:19 PM, Tim Hockin ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In examples/defaulter-gen/Makefile <#55 (comment)>: > @@ -0,0 +1,15 @@ +TOOL=defaulter-gen + +test: + @if ! git diff --quiet HEAD; then \ + echo "FAIL: git client is not clean"; \ + false; \ + fi + @go build -o /tmp/$(TOOL) + /tmp/$(TOOL) --logtostderr --v=6 -i ./output_tests/wholepkg -O zz_generated + @if ! diff -q ./output_tests/wholepkg/zz_generated.expected ./output_tests/wholepkg/zz_generated.go; then \ blech wrt having external deps. OK. Can you comment that? The point of checking it in is so that a human can look at a series of trivial output tests, and comprehend the output. --verify-only doesn't give me that? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#55 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAngljpLfmvjHoq5Slw08iwYheiUnHkiks5r69TwgaJpZM4NZ3KJ> .

thockin · 2017-05-18T05:27:46Z

In the deepcopy case, we regenerate and `git diff` which makes it easy for people to produce the thing they need to check in. In this case, it doesn't compile (we need to fix that, sigh...) so the same trick doesn't work. We need a simple way for anyone hacking to regen that code for submit. On Wed, May 17, 2017 at 10:22 PM, Daniel Smith <notifications@github.com> wrote:

…

--verify-only tells you that the checked-in thing is still indeed what the generator would output. On Wed, May 17, 2017 at 10:19 PM, Tim Hockin ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In examples/defaulter-gen/Makefile > <#55 (comment)>: > > > @@ -0,0 +1,15 @@ > +TOOL=defaulter-gen > + > +test: > + @if ! git diff --quiet HEAD; then \ > + echo "FAIL: git client is not clean"; \ > + false; \ > + fi > + @go build -o /tmp/$(TOOL) > + /tmp/$(TOOL) --logtostderr --v=6 -i ./output_tests/wholepkg -O zz_generated > + @if ! diff -q ./output_tests/wholepkg/zz_generated.expected ./output_tests/wholepkg/zz_generated.go; then \ > > blech wrt having external deps. OK. Can you comment that? > > The point of checking it in is so that a human can look at a series of > trivial output tests, and comprehend the output. --verify-only doesn't give > me that? > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#55 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/ AAngljpLfmvjHoq5Slw08iwYheiUnHkiks5r69TwgaJpZM4NZ3KJ> > . > — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#55 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFVgVDo7a3KzsrlasRIq9wYdwZmcRFBSks5r69WogaJpZM4NZ3KJ> .

caesarxuchao · 2017-05-18T06:13:13Z

How about split to 'make all' and 'make verify'?

I still need to bypass the compile issue, how about the "vendor a mock apimachinery package" trick?

mbohlool · 2017-05-18T17:24:40Z

@thockin

In the deepcopy case, we regenerate and git diff which makes it easy for
people to produce the thing they need to check in.

I disagree with this strategy for tests. Being able to check in a different test output to pass the test make it easier for people to bypass tests and don't look at the change before checking in. When people forced to fix test cases manually it more likely for them to spot a problem vs just run the test and check in the diff. (I am arguing this only for test, for production generated code, easier to check in is better).

caesarxuchao · 2017-05-18T21:22:19Z

@mbohlool i think it's fine as long as the difference of the generated code is shown in the PR and gets reviewed, so I agree with @thockin here.

@thockin How about split to 'make' to 'make all' and 'make verify', and use the '--verify-only' flag when run 'make verify'?

I still need to bypass the compile issue, how about the "vendor a mock apimachinery package" trick?

thockin · 2017-05-30T05:32:44Z

I don't care much - I just want it to fail in an obvious way so humans can examine the changes.

…

On Thu, May 18, 2017 at 2:22 PM, Chao Xu ***@***.***> wrote: @mbohlool <https://github.com/mbohlool> i think it's fine as long as the difference of the generated code is shown in the PR and gets reviewed, so I agree with @thockin <https://github.com/thockin> here. @thockin <https://github.com/thockin> How about split to 'make' to 'make all' and 'make verify', and use the '--verify-only' flag when run 'make verify'? I still need to bypass the compile issue, how about the "vendor a mock apimachinery package" trick? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#55 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFVgVLlsTnOrKF3pddylYeuGPQSjaY9Qks5r7LaLgaJpZM4NZ3KJ> .

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 12, 2017

mbohlool reviewed May 15, 2017

View reviewed changes

caesarxuchao force-pushed the add-defaulter-gen-tests branch 2 times, most recently from 29bd77c to 483cca9 Compare May 15, 2017 23:55

caesarxuchao force-pushed the add-defaulter-gen-tests branch from 483cca9 to b289754 Compare May 16, 2017 00:03

thockin reviewed May 17, 2017

View reviewed changes

add tests for default-gen

e0660ff

caesarxuchao force-pushed the add-defaulter-gen-tests branch from b289754 to e0660ff Compare May 17, 2017 19:39

caesarxuchao mentioned this pull request May 30, 2017

Default-gen changes for moving API types to k8s.io/api #54

Merged

nikhita mentioned this pull request Jun 12, 2017

Fix defaulter-gen for recursive types #61

Merged

caesarxuchao closed this Jul 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add a test for default-gen output #55

add a test for default-gen output #55

caesarxuchao commented May 12, 2017 •

edited

Loading

k8s-reviewable commented May 12, 2017

caesarxuchao commented May 12, 2017

mbohlool May 15, 2017

caesarxuchao May 15, 2017

mbohlool May 15, 2017

caesarxuchao May 15, 2017

mbohlool May 15, 2017

caesarxuchao May 15, 2017

caesarxuchao May 15, 2017

mbohlool May 15, 2017

caesarxuchao commented May 15, 2017

mbohlool commented May 15, 2017

mbohlool commented May 16, 2017

thockin May 17, 2017

caesarxuchao May 17, 2017

lavalamp May 17, 2017

caesarxuchao May 17, 2017 •

edited

Loading

thockin May 18, 2017

thockin May 17, 2017

caesarxuchao May 17, 2017

lavalamp commented May 18, 2017 via email

thockin commented May 18, 2017 via email

caesarxuchao commented May 18, 2017

mbohlool commented May 18, 2017 •

edited

Loading

caesarxuchao commented May 18, 2017

thockin commented May 30, 2017 via email

add a test for default-gen output #55

add a test for default-gen output #55

Conversation

caesarxuchao commented May 12, 2017 • edited Loading

k8s-reviewable commented May 12, 2017

caesarxuchao commented May 12, 2017

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

Choose a reason for hiding this comment

caesarxuchao commented May 15, 2017

mbohlool commented May 15, 2017

mbohlool commented May 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesarxuchao May 17, 2017 • 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

lavalamp commented May 18, 2017 via email

thockin commented May 18, 2017 via email

caesarxuchao commented May 18, 2017

mbohlool commented May 18, 2017 • edited Loading

caesarxuchao commented May 18, 2017

thockin commented May 30, 2017 via email

caesarxuchao commented May 12, 2017 •

edited

Loading

caesarxuchao May 17, 2017 •

edited

Loading

mbohlool commented May 18, 2017 •

edited

Loading