Run KafkaSource smokes in upgrade test #524

cardil · 2021-04-08T14:56:22Z

Partially resolves #67

Proposed Changes

Run pre, post smoke tests for KafkaSource on upgrade tests

Continual tests will follow to fully resolve #67

codecov · 2021-04-08T18:15:47Z

Codecov Report

Merging #524 (5302956) into main (bc84841) will decrease coverage by 0.01%.
The diff coverage is 40.90%.

@@            Coverage Diff             @@
##             main     #524      +/-   ##
==========================================
- Coverage   72.87%   72.86%   -0.02%     
==========================================
  Files         147      147              
  Lines        5825     5856      +31     
==========================================
+ Hits         4245     4267      +22     
- Misses       1335     1344       +9     
  Partials      245      245

Impacted Files	Coverage Δ
test/upgrade/smoke.go	`0.00% <0.00%> (ø)`
test/upgrade/continual.go	`16.27% <33.33%> (+2.76%)`	⬆️
test/upgrade/postdowngrade.go	`33.33% <33.33%> (ø)`
test/upgrade/postupgrade.go	`33.33% <33.33%> (ø)`
test/upgrade/preupgrade.go	`33.33% <33.33%> (ø)`
test/upgrade/suite.go	`100.00% <100.00%> (ø)`
.../channel/distributed/receiver/producer/producer.go	`69.23% <0.00%> (-0.34%)`	⬇️
pkg/source/mtadapter/adapter.go	`70.45% <0.00%> (+0.33%)`	⬆️
...el/distributed/dispatcher/dispatcher/dispatcher.go	`74.64% <0.00%> (+0.36%)`	⬆️
pkg/source/adapter/adapter.go	`60.49% <0.00%> (+1.51%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bc84841...5302956. Read the comment docs.

cardil · 2021-04-08T18:35:05Z

test/e2e/kafka_source_test.go

-//+build e2e mtsource
-//+build source
+// +build e2e
+// +build source mtsource


I think this is more correct.

It means: e2e AND (source OR mtsource).

Previous have meant: (e2e or mtsource) AND source, which is weird.

lionelvillard · 2021-04-08T20:26:56Z

test/e2e-common.sh

@@ -315,12 +315,40 @@ function install_consolidated_channel_crds {
  wait_until_pods_running "${SYSTEM_NAMESPACE}"
 }

+function install_released_consolidated_source {


I would remove consolidated. Just install_released_source.

I suppose the upgrade tests are black-box tests, in which case they can also run against the multi-tenant source variant.

The function name contained consolidated word before, that's why I didn't remove it.

That's right those tests should be a black box, but I believe future conditional logic should be in golang in installation package rather than bash scripts.

My two cents: After reading install_released_consolidated_source my first question is - what "source" ? It could be "source code". Maybe we could say install_released_event_source instead.

mgencur

Looks very good! Just a few comments here and there

mgencur · 2021-04-09T07:44:08Z

test/e2e-common.sh

@@ -315,12 +315,40 @@ function install_consolidated_channel_crds {
  wait_until_pods_running "${SYSTEM_NAMESPACE}"
 }

+function install_released_consolidated_source {


My two cents: After reading install_released_consolidated_source my first question is - what "source" ? It could be "source code". Maybe we could say install_released_event_source instead.

test/e2e/kafka_source.go

mgencur · 2021-04-09T07:52:06Z

test/upgrade/continual.go

+// well.
+func SourceContinualTest() pkgupgrade.BackgroundOperation {
+	setup := func(c pkgupgrade.Context) {
+		// TODO: not yet implemented


I suppose this comment is redundant? Same for the comment a few lines below

I'd leave it be. The // TODO: is reported by IDE and other dev tools. Warn log is useful on logs. That's why it's repeated.

mgencur · 2021-04-09T07:53:54Z

test/upgrade/postdowngrade.go

@@ -28,3 +28,11 @@ func ChannelPostDowngradeTest() pkgupgrade.Operation {
 			runChannelSmokeTest(c.T)
 		})
 }
+
+// SourcePostDowngradeTest tests source operations after downgrade.
+func SourcePostDowngradeTest() pkgupgrade.Operation {


I don't know but does it make sense to rename all these functions to have the Event prefix?
E.g. EventSourceDowngradeTest

This repo contains Kafka things. KafkaSource and KafkaChannel. I'm shorting Kafka, but not only I. That naming was already in this repo before. Let's not create new names like enentsource.

It looks like these two (Source and KafkaSource) are totally mixed in function names in this repo. E.g. runSourceSmokeTest vs. AssureKafkaSourceIsOperational vs. testKafkaSource vs. SourcePostDowngradeTest
That's a cosmetic thing, having just "Source" is a bit more misleading to me. Nevermind

aliok · 2021-04-09T08:12:33Z

Looks very good to me :)
Thanks!

Leaving the merge to @mgencur

Co-authored-by: Martin Gencur <[email protected]>

aliok · 2021-04-09T09:12:19Z

/approve

mgencur · 2021-04-09T09:14:00Z

/retest

mgencur · 2021-04-09T09:14:05Z

/lgtm

cardil · 2021-04-09T09:37:04Z

/retest

cardil · 2021-04-09T10:21:01Z

/woof

knative-prow-robot · 2021-04-09T10:21:02Z

@cardil:

In response to this:

/woof

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.

cardil · 2021-04-09T10:23:19Z

/joke

knative-prow-robot · 2021-04-09T10:23:20Z

@cardil: What concert costs only 45 cents? 50 cent featuring Nickelback.

In response to this:

/joke

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.

cardil · 2021-04-09T11:06:24Z

/retest

mgencur · 2021-04-09T11:28:46Z

/test all

aliok

/lgtm
/approve

knative-prow-robot · 2021-04-09T11:35:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aliok

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

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

* Run KafkaSource smokes in upgrade test * Fixing unit tests * Restore upgrade tests tags * Fix codegen * Reformat README.md * Golangci Link fix * Simplify godoc SourceTestScope Conflicts fixed: * test/e2e-common.sh * test/e2e/kafka_source_test.go

…tensions#524) Run KafkaSource smokes in upgrade test (knative-extensions#524) * Run KafkaSource smokes in upgrade test * Fixing unit tests * Restore upgrade tests tags * Fix codegen * Reformat README.md * Golangci Link fix * Simplify godoc SourceTestScope Conflicts fixed: * test/e2e-common.sh

* Run KafkaSource smokes in upgrade test * Fixing unit tests * Restore upgrade tests tags * Fix codegen * Reformat README.md * Golangci Link fix * Simplify godoc SourceTestScope Co-authored-by: Martin Gencur <[email protected]> Co-authored-by: Martin Gencur <[email protected]>

* Run KafkaSource smokes in upgrade test * Fixing unit tests * Restore upgrade tests tags * Fix codegen * Reformat README.md * Golangci Link fix * Simplify godoc SourceTestScope Co-authored-by: Martin Gencur <[email protected]> Co-authored-by: Martin Gencur <[email protected]> Co-authored-by: Martin Gencur <[email protected]>

…tensions#524) (#150) Run KafkaSource smokes in upgrade test (knative-extensions#524) * Run KafkaSource smokes in upgrade test * Fixing unit tests * Restore upgrade tests tags * Fix codegen * Reformat README.md * Golangci Link fix * Simplify godoc SourceTestScope Conflicts fixed: * test/e2e-common.sh

cardil requested review from a team as code owners April 8, 2021 14:56

cardil requested a review from a user April 8, 2021 14:56

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Apr 8, 2021

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Apr 8, 2021

Run KafkaSource smokes in upgrade test

2fae962

cardil force-pushed the feature/kafka-source-upgrade-tests-p1 branch from 85af36a to 2fae962 Compare April 8, 2021 15:00

Fixing unit tests

f191623

Restore upgrade tests tags

6c42188

cardil commented Apr 8, 2021

View reviewed changes

cardil added 3 commits April 8, 2021 20:43

Fix codegen

056f356

Reformat README.md

602d9f9

Golangci Link fix

4071339

lionelvillard reviewed Apr 8, 2021

View reviewed changes

mgencur reviewed Apr 9, 2021

View reviewed changes

Simplify godoc SourceTestScope

5302956

Co-authored-by: Martin Gencur <[email protected]>

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 9, 2021

knative-prow-robot assigned mgencur Apr 9, 2021

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 9, 2021

aliok approved these changes Apr 9, 2021

View reviewed changes

knative-prow-robot assigned aliok Apr 9, 2021

knative-prow-robot merged commit 148b944 into knative-extensions:main Apr 9, 2021

cardil deleted the feature/kafka-source-upgrade-tests-p1 branch April 9, 2021 12:10

cardil mentioned this pull request Apr 9, 2021

[0.20] Backport of PR #524 Run KafkaSource smokes in upgrade test openshift-knative/eventing-kafka#143

Merged

cardil mentioned this pull request Apr 12, 2021

[0.21] Backport of Run KafkaSource smokes in upgrade test (knative-sandbox#524) openshift-knative/eventing-kafka#150

Merged

cardil mentioned this pull request Apr 12, 2021

[0.22] Backport of Run KafkaSource smokes in upgrade test (knative-sandbox#524) openshift-knative/eventing-kafka#151

Merged

pierDipi mentioned this pull request Oct 4, 2021

Avoid waiting for offsets initialization in dispatcher openshift-knative/eventing-kafka#382

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run KafkaSource smokes in upgrade test #524

Run KafkaSource smokes in upgrade test #524

cardil commented Apr 8, 2021

codecov bot commented Apr 8, 2021 •

edited

Loading

cardil Apr 8, 2021

lionelvillard Apr 8, 2021

cardil Apr 9, 2021

mgencur Apr 9, 2021

mgencur left a comment

mgencur Apr 9, 2021

mgencur Apr 9, 2021

cardil Apr 9, 2021

mgencur Apr 9, 2021

cardil Apr 9, 2021

mgencur Apr 9, 2021

aliok commented Apr 9, 2021

aliok commented Apr 9, 2021

mgencur commented Apr 9, 2021

mgencur commented Apr 9, 2021

cardil commented Apr 9, 2021

cardil commented Apr 9, 2021

knative-prow-robot commented Apr 9, 2021

cardil commented Apr 9, 2021

knative-prow-robot commented Apr 9, 2021

cardil commented Apr 9, 2021

mgencur commented Apr 9, 2021

aliok left a comment

knative-prow-robot commented Apr 9, 2021

Run KafkaSource smokes in upgrade test #524

Run KafkaSource smokes in upgrade test #524

Conversation

cardil commented Apr 8, 2021

Proposed Changes

codecov bot commented Apr 8, 2021 • edited Loading

Codecov Report

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

mgencur 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

Choose a reason for hiding this comment

aliok commented Apr 9, 2021

aliok commented Apr 9, 2021

mgencur commented Apr 9, 2021

mgencur commented Apr 9, 2021

cardil commented Apr 9, 2021

cardil commented Apr 9, 2021

knative-prow-robot commented Apr 9, 2021

cardil commented Apr 9, 2021

knative-prow-robot commented Apr 9, 2021

cardil commented Apr 9, 2021

mgencur commented Apr 9, 2021

aliok left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Apr 9, 2021

codecov bot commented Apr 8, 2021 •

edited

Loading