add roundtrip tests #3574

vaikas · 2020-07-13T22:30:48Z

Add RoundTrip tests for eventing., messaging., and flows.* resources.

Proposed Changes

Add round trip tests and fuzzer for eventing., messaging., and flows.* resources.

Release Note

- 🐛 Fix bug
DeadLetterChannel was being dropped when converting between v1beta1<->v1
Not all the conditions were being properly converted between v1beta1<->v1. Basically only the Ready was.

Docs

vaikas · 2020-07-13T22:31:00Z

@dprotaso would you mind taking a looksie?

knative-prow-robot · 2020-07-13T22:31:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vaikas

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~~ [vaikas]

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

dprotaso · 2020-07-14T20:01:52Z

Looks good - you may want to also test round tripping between your hub version and your v1 types

ie.
https://github.com/knative/serving/blob/05db8e3b9b7d81807b35820d61ea2e0b26daccbe/pkg/apis/serving/v1alpha1/roundtrip_test.go#L42-L67

lgtm-com · 2020-07-14T22:52:13Z

This pull request introduces 1 alert when merging 6e8afa1 into 54e191e - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

dprotaso · 2020-07-15T16:00:56Z

pkg/apis/duck/v1/fuzzer.go

+var FuzzerFuncs = fuzzer.MergeFuzzerFuncs(
+	func(codecs serializer.CodecFactory) []interface{} {
+		return []interface{}{
+			func(ds *DeliverySpec, c fuzz.Continue) {


This means it won’t fuzz the other fields of the delivery spec - you may want to fuzz it here (no custom) and just override the back off policy after

dprotaso · 2020-07-15T16:03:15Z

pkg/apis/duck/v1beta1/delivery_conversion_test.go

+}
+
+// Test v1 -> v1beta1 -> v1
+func TestDeliveryStatuscConversionV1(t *testing.T) {


Typo in func name

Also to confirm you wrote this test so that avoid side-effects failure on the input?

No, that was covered by the RT tests. I added these because we really didn't have any tests, so wanted to increase coverage.

dprotaso · 2020-07-15T16:04:49Z

I think there are funcs names with other typos

dprotaso · 2020-07-15T16:26:41Z

pkg/apis/messaging/v1/fuzzer.go

+				}
+			},
+			func(imc *InMemoryChannel, c fuzz.Continue) {
+				if imc != nil {


likewise - i this won't fuzz the other fields so you'll need FuzzNoCustom on imc

ack, also combined the status here so don't need both IMC + IMC.Status. Is that right? I think it is because it uncovered a bug :)

also combined the status here so don't need both IMC + IMC.Status. Is that right?

FuzzNoCustom won't call these supplied fuzzer functions so you'll have to fuzz it yourself. You could use c.Fuzz for nested types if you did need to customize the fuzzing.

Ack, so, is it now right?

FuzzNoCustom won't call these supplied fuzzer functions so you'll have to fuzz it yourself. You could use c.Fuzz for nested types if you did need to customize the fuzzing.

I was wrong about this - by bad

dprotaso · 2020-07-15T16:26:50Z

pkg/apis/messaging/v1/fuzzer.go

+var FuzzerFuncs = fuzzer.MergeFuzzerFuncs(
+	func(codecs serializer.CodecFactory) []interface{} {
+		return []interface{}{
+			func(ch *Channel, c fuzz.Continue) {


likewise - i this won't fuzz the other fields so you'll need FuzzNoCustom on ch

vaikas · 2020-07-15T17:56:56Z

Thanks @dprotaso !! I think I addressed all the comments, and found / fixed a bug :)

pkg/apis/duck/v1beta1/delivery_conversion_test.go

knative-metrics-robot · 2020-07-15T18:31:15Z

The following is the coverage report on the affected files.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/duck/v1/fuzzer.go	Do not exist	0.0%
pkg/apis/duck/v1beta1/delivery_conversion.go	70.6%	100.0%	29.4
pkg/apis/eventing/v1/fuzzer.go	Do not exist	100.0%
pkg/apis/flows/v1/fuzzer.go	Do not exist	100.0%
pkg/apis/messaging/v1/fuzzer.go	Do not exist	100.0%
pkg/apis/messaging/v1beta1/in_memory_channel_conversion.go	97.4%	97.2%	-0.1

knative-prow-robot · 2020-07-15T18:31:32Z

@vaikas: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-knative-eventing-go-coverage	`a5ae3fa`	link	`/test pull-knative-eventing-go-coverage`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

dprotaso · 2020-07-15T19:01:44Z

/lgtm

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 13, 2020

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 13, 2020

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 13, 2020

knative-prow-robot requested review from Harwayne and matzew July 13, 2020 22:31

add roundtrip tests / fuzzer

447a352

vaikas force-pushed the eventing-fuzz branch from 243ef35 to 447a352 Compare July 13, 2020 22:37

add RT tests round tripping via hub

6e8afa1

clean the code, add moar ut for delivery conversion

6563cb4

dprotaso reviewed Jul 15, 2020

View reviewed changes

fix rt bug, fuzz all the fields

e7ebef7

dprotaso reviewed Jul 15, 2020

View reviewed changes

pkg/apis/duck/v1beta1/delivery_conversion_test.go Outdated Show resolved Hide resolved

fix typo

a5ae3fa

knative-prow-robot assigned dprotaso Jul 15, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2020

knative-prow-robot merged commit 4f2f82d into knative:master Jul 15, 2020

vaikas deleted the eventing-fuzz branch July 16, 2020 20:52

vaikas mentioned this pull request Jul 18, 2020

Pingsource v1beta1 #3607

Merged

vaikas mentioned this pull request Oct 22, 2020

DeepCopy the entire set of conditions knative-extensions/eventing-kafka#128

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add roundtrip tests #3574

add roundtrip tests #3574

vaikas commented Jul 13, 2020 •

edited

Loading

vaikas commented Jul 13, 2020

knative-prow-robot commented Jul 13, 2020

dprotaso commented Jul 14, 2020

lgtm-com bot commented Jul 14, 2020

dprotaso Jul 15, 2020

dprotaso Jul 15, 2020

vaikas Jul 15, 2020

dprotaso commented Jul 15, 2020

dprotaso Jul 15, 2020

vaikas Jul 15, 2020

dprotaso Jul 15, 2020

vaikas Jul 15, 2020

dprotaso Jul 15, 2020

dprotaso Jul 15, 2020

vaikas commented Jul 15, 2020

knative-metrics-robot commented Jul 15, 2020

knative-prow-robot commented Jul 15, 2020

dprotaso commented Jul 15, 2020

add roundtrip tests #3574

add roundtrip tests #3574

Conversation

vaikas commented Jul 13, 2020 • edited Loading

Proposed Changes

vaikas commented Jul 13, 2020

knative-prow-robot commented Jul 13, 2020

dprotaso commented Jul 14, 2020

lgtm-com bot commented Jul 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dprotaso commented Jul 15, 2020

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

vaikas commented Jul 15, 2020

knative-metrics-robot commented Jul 15, 2020

knative-prow-robot commented Jul 15, 2020

dprotaso commented Jul 15, 2020

vaikas commented Jul 13, 2020 •

edited

Loading