Add fuzz tests missing vendor changes #1306

pooneh-m · 2020-01-30T01:21:33Z

vendor_fixes is missing fuzz testing dependencies.
#1098

pooneh-m · 2020-01-30T01:23:29Z

I am not sure how it was working before, but I assume there were local changes that never got pushed? Or I am missing something.

agones-bot · 2020-01-30T01:34:23Z

Build Succeeded 👏

Build Id: 7d2a9b1a-3b13-4fb3-bbe2-be0389950ea8

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.4.0-24738fc
image: gcr.io/agones-images/agones-sdk:1.4.0-24738fc
image: gcr.io/agones-images/agones-ping:1.4.0-24738fc
Linux C++ SDK (build): agonessdk-1.4.0-24738fc-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.4.0-24738fc.zip

A preview of the website (the last 30 builds are retained):

https://24738fc-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1306/head:pr_1306 && git checkout pr_1306
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.4.0-24738fc

roberthbailey · 2020-01-30T02:19:15Z

/assign @aLekSer

roberthbailey · 2020-01-30T02:19:29Z

@aLekSer - is there a reason that these files weren't included in #1269?

aLekSer · 2020-01-30T13:32:45Z

@roberthbailey We have all this files in a vendor directory, so it seems that we don't need to add duplicate files into vendor_fixes:

 git ls-files ../vendor/k8s.io/apimachinery/pkg/api/validation/
../vendor/k8s.io/apimachinery/pkg/api/validation/doc.go
../vendor/k8s.io/apimachinery/pkg/api/validation/generic.go
../vendor/k8s.io/apimachinery/pkg/api/validation/objectmeta.go
../vendor/k8s.io/apimachinery/pkg/api/validation/objectmeta_test.go
../vendor/k8s.io/apimachinery/pkg/api/validation/path/name.go
../vendor/k8s.io/apimachinery/pkg/api/validation/path/name_test.go

8 files here:

git ls-files ../vendor/k8s.io/apimachinery/pkg/api/apitesting/ | wc -l
       5
 git ls-files ../vendor/k8s.io/apimachinery/pkg/api/apitesting/fuzzer/  | wc -l 
       3

aLekSer

We have all the files in a ./vendor/ directory, I can run diff for all that files. It seems that we could skip this PR as files are already in the repo.

aLekSer · 2020-01-30T13:57:22Z

Found a diff in roundtrip.go file, I will check the versions, which you are adding and which was added to ./vendor:

diff ~/go/src/agones.dev/agones/vendor/k8s.io/apimachinery/pkg/api/apitesting/roundtrip/roundtrip.go  ~/go/src/agones.dev/agones/vendor_fixes/k8s.io/apimachinery/pkg/api/apitesting//roundtrip/roundtrip.go
143,149c143,145
<
<               // FIXME: this is explicitly testing w/o protobuf which was failing if enabled
<               // the reason for that is that protobuf is not setting Kind and APIVersion fields
<               // during obj2 decode, the same then applies to DecodeInto obj3. My guess is we
<               // should be setting these two fields accordingly when protobuf is passed as codec
<               // to roundTrip method.
<               roundTripSpecificKind(t, gvk, scheme, codecFactory, fuzzer, nonRoundTrippableTypes, true)                                                                                                
---
>               t.Run(gvk.Group+"."+gvk.Version+"."+gvk.Kind, func(t *testing.T) {
>                       roundTripSpecificKind(t, gvk, scheme, codecFactory, fuzzer, nonRoundTrippableTypes, false)                                                                                       
>               })
166d161
<       t.Logf("round tripping %v", gvk)
234c229
<                       s := protobuf.NewSerializer(scheme, scheme, "application/arbitrary.content.type")                                                                                                
---
>                       s := protobuf.NewSerializer(scheme, scheme)
253,255d247
<       externalGoType := reflect.TypeOf(object).PkgPath()
<       t.Logf("\tround tripping external type %v %v", externalGVK, externalGoType)
<
263c255
<               roundTrip(t, scheme, protobuf.NewSerializer(scheme, scheme, "application/protobuf"), object)                                                                                             
---
>               roundTrip(t, scheme, protobuf.NewSerializer(scheme, scheme), object)

pooneh-m · 2020-01-30T17:33:13Z

If I understand correctly vendor_fixes/k8s.io/apimachinery replaces vendor/k8s.io/apimachinery: https://github.com/googleforgames/agones/blob/master/go.mod#L61

So we should have a copy of apitesting in vendor_fixes, correct?

aLekSer · 2020-01-30T20:05:24Z

First of all, I think we should approve this PR with the requirement to copy those files to vendor as wel (using go mod vendor command, which resets the main module's vendor directory). You are correct, and I have broken the vendor_fixes as it can be seen from this output.

go mod vendor -v
~/go/src/agones.dev/agones/test/sdk/nodejs/node_modules/agones                                                                                         
go: finding k8s.io/apimachinery v0.17.2
agones.dev/agones/pkg/apis/agones/v1 imports
        k8s.io/apimachinery/pkg/api/validation: module k8s.io/apimachinery@latest (v0.17.2) found, but does not contain package k8s.io/apimachinery/pkg/api/validation                                   
agones.dev/agones/pkg/apis/agones/v1 imports
        k8s.io/apimachinery/pkg/apis/meta/v1/validation: module k8s.io/apimachinery@latest (v0.17.2) found, but does not contain package k8s.io/apimachinery/pkg/apis/meta/v1/validation

Second is that statement taken from Go cmd tips. It states that with go test -mod=vendor we stop using network and mod cache and begin to use vendor itself.

To build using the main module's top-level vendor directory to satisfy dependencies (disabling use of the usual network sources and local caches), use 'go build -mod=vendor'. Note that only the main module's top-level vendor directory is used; vendor directories in other locations are still ignored

aLekSer

Left some comments, but overall I am waiting when we would remove vendor_fixes which are confusing.

aLekSer · 2020-01-30T20:07:51Z

vendor_fixes/k8s.io/apimachinery/pkg/api/apitesting/roundtrip/roundtrip.go

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


Please add this and other files to a vendor directory as well. This file has diff with what I have added to vendor. Please use the version which is taken from k8s.io/apimachinery v0.0.0-20191004074956-01f8b7d1121a

I am not sure if I understand this comment. Are you saying apimachinary copy in vendor is not the same as the version specified in mod?

When I run go mod vendor roundtrip.go doesn't change. However, there are other changes:

deleted: vendor/github.com/google/btree/btree_mem.go deleted: vendor/golang.org/x/sys/unix/mkasm_darwin.go deleted: vendor/golang.org/x/sys/unix/mkpost.go deleted: vendor/golang.org/x/sys/unix/mksyscall.go deleted: vendor/golang.org/x/sys/unix/mksyscall_aix_ppc.go deleted: vendor/golang.org/x/sys/unix/mksyscall_aix_ppc64.go deleted: vendor/golang.org/x/sys/unix/mksyscall_solaris.go deleted: vendor/golang.org/x/sys/unix/mksysnum.go deleted: vendor/golang.org/x/sys/unix/types_aix.go deleted: vendor/golang.org/x/sys/unix/types_darwin.go deleted: vendor/golang.org/x/sys/unix/types_dragonfly.go deleted: vendor/golang.org/x/sys/unix/types_freebsd.go deleted: vendor/golang.org/x/sys/unix/types_netbsd.go deleted: vendor/golang.org/x/sys/unix/types_openbsd.go deleted: vendor/golang.org/x/sys/unix/types_solaris.go deleted: vendor/golang.org/x/text/unicode/bidi/gen.go deleted: vendor/golang.org/x/text/unicode/bidi/gen_ranges.go deleted: vendor/golang.org/x/text/unicode/bidi/gen_trieval.go deleted: vendor/golang.org/x/text/unicode/norm/maketables.go deleted: vendor/golang.org/x/text/unicode/norm/triegen.go deleted: vendor/golang.org/x/text/width/gen.go deleted: vendor/golang.org/x/text/width/gen_common.go deleted: vendor/golang.org/x/text/width/gen_trieval.go deleted: vendor/golang.org/x/tools/go/gcexportdata/main.go deleted: vendor/golang.org/x/tools/imports/mkindex.go deleted: vendor/golang.org/x/tools/imports/mkstdlib.go deleted: vendor/k8s.io/apimachinery/pkg/api/apitesting/fuzzer/valuefuzz_test.go deleted: vendor/k8s.io/apimachinery/pkg/api/validation/objectmeta_test.go deleted: vendor/k8s.io/apimachinery/pkg/api/validation/path/name.go deleted: vendor/k8s.io/apimachinery/pkg/api/validation/path/name_test.go deleted: vendor/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation_test.go

aLekSer · 2020-01-30T22:20:16Z

I mean difference between vendor and vendor_fixes for apimachinery they should be equal. Thought go mod vendor would create missing files and also fixed that difference.

pooneh-m · 2020-01-31T00:02:23Z

I mean difference between vendor and vendor_fixes for apimachinery they should be equal. Thought go mod vendor would create missing files and also fixed that difference.

Exactly, with this PR vendor_fix has the copy of required files for round trip fuzz testing from vendor. The vendor is based on k8s.io/apimachinery v0.0.0-20191004074956-01f8b7d1121a

aLekSer

Verified the diff between vendor/k8s.io/apimachinery directory and vendor_fixes one. Now I see that they contains equal files, except for an absence of go.mod file in vendor/k8s.io/apimachinery.

aLekSer · 2020-01-31T17:41:32Z

One more thing I have found is:

go mod tidy                                                                                      
agones.dev/agones/cmd/allocator imports
        k8s.io/client-go/rest tested by
        k8s.io/client-go/rest.test imports
        k8s.io/apimachinery/pkg/util/httpstream: module k8s.io/apimachinery@latest (v0.17.2) found, but does not contain package k8s.io/apimachinery/pkg/util/httpstream

With no directory in the path ./vendor_fixes/k8s.io/apimachinery/pkg/util/httpstream
I think we could add them as part of this PR to be consistent.

pooneh-m · 2020-01-31T22:07:41Z

One more thing I have found is:
go mod tidy                                                                                      
agones.dev/agones/cmd/allocator imports
        k8s.io/client-go/rest tested by
        k8s.io/client-go/rest.test imports
        k8s.io/apimachinery/pkg/util/httpstream: module k8s.io/apimachinery@latest (v0.17.2) found, but does not contain package k8s.io/apimachinery/pkg/util/httpstream    
With no directory in the path ./vendor_fixes/k8s.io/apimachinery/pkg/util/httpstream
I think we could add them as part of this PR to be consistent.

Good find. There is no k8s.io/apimachinery/pkg/util/httpstream to copy over. We need to upgrade apimachinery. I created this issue #1309 to track that.

agones-bot · 2020-01-31T22:23:50Z

Build Succeeded 👏

Build Id: a6811577-98d1-486e-b04b-4e5366e99bf3

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.4.0-4e1b5dd
image: gcr.io/agones-images/agones-sdk:1.4.0-4e1b5dd
image: gcr.io/agones-images/agones-ping:1.4.0-4e1b5dd
Linux C++ SDK (build): agonessdk-1.4.0-4e1b5dd-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.4.0-4e1b5dd.zip

A preview of the website (the last 30 builds are retained):

https://4e1b5dd-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1306/head:pr_1306 && git checkout pr_1306
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.4.0-4e1b5dd

roberthbailey

I verified that there are no diffs between any of the files added here to vendor_fixes from the contents of the files in the vendor directory, so I'm going to merge this to unblock @pooneh-m from testing and debugging. We can fix the go mod tidy issues in a follow-up PR.

google-oss-robot · 2020-01-31T23:06:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aLekSer, pooneh-m, roberthbailey

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [pooneh-m,roberthbailey]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Add fuzz tests missing vendor changes

24738fc

google-oss-robot requested review from dzlier-gcp and EricFortin January 30, 2020 01:21

google-oss-robot added approved size/XXL labels Jan 30, 2020

pooneh-m requested review from aLekSer and roberthbailey and removed request for EricFortin and dzlier-gcp January 30, 2020 01:23

google-oss-robot assigned aLekSer Jan 30, 2020

aLekSer suggested changes Jan 30, 2020

View reviewed changes

aLekSer reviewed Jan 30, 2020

View reviewed changes

pooneh-m mentioned this pull request Jan 30, 2020

Fix deep copy for multi-cluster allocation policy CRD #1308

Merged

aLekSer approved these changes Jan 31, 2020

View reviewed changes

google-oss-robot added the lgtm label Jan 31, 2020

Merge branch 'master' into fuzz_fix

4e1b5dd

google-oss-robot removed the lgtm label Jan 31, 2020

roberthbailey approved these changes Jan 31, 2020

View reviewed changes

google-oss-robot assigned roberthbailey Jan 31, 2020

google-oss-robot added the lgtm label Jan 31, 2020

roberthbailey merged commit f9469ef into googleforgames:master Jan 31, 2020

markmandel added this to the 1.4.0 milestone Feb 26, 2020

markmandel added area/tests Unit tests, e2e tests, anything to make sure things don't break kind/feature New features for Agones labels Feb 26, 2020

ilkercelikyilmaz pushed a commit to ilkercelikyilmaz/agones that referenced this pull request Oct 23, 2020

Add fuzz tests missing vendor changes (googleforgames#1306)

692c6ab

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fuzz tests missing vendor changes #1306

Add fuzz tests missing vendor changes #1306

pooneh-m commented Jan 30, 2020

pooneh-m commented Jan 30, 2020 •

edited

Loading

agones-bot commented Jan 30, 2020

roberthbailey commented Jan 30, 2020

roberthbailey commented Jan 30, 2020

aLekSer commented Jan 30, 2020 •

edited

Loading

aLekSer left a comment •

edited

Loading

aLekSer commented Jan 30, 2020 •

edited

Loading

pooneh-m commented Jan 30, 2020

aLekSer commented Jan 30, 2020 •

edited

Loading

aLekSer left a comment

aLekSer Jan 30, 2020 •

edited

Loading

pooneh-m Jan 30, 2020

aLekSer commented Jan 30, 2020

pooneh-m commented Jan 31, 2020

aLekSer left a comment •

edited

Loading

aLekSer commented Jan 31, 2020 •

edited

Loading

pooneh-m commented Jan 31, 2020

agones-bot commented Jan 31, 2020

roberthbailey left a comment

google-oss-robot commented Jan 31, 2020

Add fuzz tests missing vendor changes #1306

Add fuzz tests missing vendor changes #1306

Conversation

pooneh-m commented Jan 30, 2020

pooneh-m commented Jan 30, 2020 • edited Loading

agones-bot commented Jan 30, 2020

roberthbailey commented Jan 30, 2020

roberthbailey commented Jan 30, 2020

aLekSer commented Jan 30, 2020 • edited Loading

aLekSer left a comment • edited Loading

Choose a reason for hiding this comment

aLekSer commented Jan 30, 2020 • edited Loading

pooneh-m commented Jan 30, 2020

aLekSer commented Jan 30, 2020 • edited Loading

aLekSer left a comment

Choose a reason for hiding this comment

aLekSer Jan 30, 2020 • edited Loading

Choose a reason for hiding this comment

pooneh-m Jan 30, 2020

Choose a reason for hiding this comment

aLekSer commented Jan 30, 2020

pooneh-m commented Jan 31, 2020

aLekSer left a comment • edited Loading

Choose a reason for hiding this comment

aLekSer commented Jan 31, 2020 • edited Loading

pooneh-m commented Jan 31, 2020

agones-bot commented Jan 31, 2020

roberthbailey left a comment

Choose a reason for hiding this comment

google-oss-robot commented Jan 31, 2020

pooneh-m commented Jan 30, 2020 •

edited

Loading

aLekSer commented Jan 30, 2020 •

edited

Loading

aLekSer left a comment •

edited

Loading

aLekSer commented Jan 30, 2020 •

edited

Loading

aLekSer commented Jan 30, 2020 •

edited

Loading

aLekSer Jan 30, 2020 •

edited

Loading

aLekSer left a comment •

edited

Loading

aLekSer commented Jan 31, 2020 •

edited

Loading