(refactor) address the e2e extract / refactor of issue #763 by maximilien · Pull Request #765 · knative/client

maximilien · 2020-03-27T03:11:11Z

Description

Addresses the e2e part of issue #763. Moves common code to lib/test/integration

Changes

refactors the common e2e code into lib/test/integration
change package name of the common code from e2e to integration
change all e2e test to use new common code

Reference

Partially fix to issue #763

knative-prow-robot · 2020-03-27T03:11:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maximilien

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [maximilien]

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

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

maximilien · 2020-03-27T03:22:01Z

/test pull-knative-client-build-tests

maximilien · 2020-03-27T03:22:44Z

/assign @rhuss @daisy-ycguo

navidshaikh · 2020-03-27T05:13:29Z

cause of build/e2e tests failure

test/e2e/service_export_import_apply_test.go:39:15: undefined: NewE2eTest
test/e2e/service_export_import_apply_test.go:45:7: undefined: NewKnRunResultCollector
test/e2e/service_export_import_apply_test.go:69:13: undefined: e2eTest
test/e2e/service_export_import_apply_test.go:69:53: undefined: KnRunResultCollector
test/e2e/service_export_import_apply_test.go:84:13: undefined: e2eTest
test/e2e/service_export_import_apply_test.go:84:66: undefined: KnRunResultCollector
test/e2e/service_export_import_apply_test.go:111:21: undefined: KnDefaultTestImage
test/e2e/trigger_test.go:63:2: undefined: test
test/e2e/trigger_test.go:64:2: undefined: test
test/e2e/trigger_test.go:65:2: undefined: test
test/e2e/trigger_test.go:65:2: too many errors

navidshaikh · 2020-03-27T06:09:15Z

 */

-package e2e
+package integration


IMO the package name could simply be e2e ?

imports like lib/test/e2e/flags

or even shorter lib/e2e/flags

agree with e2e for short, other than integration. just feel too long.
If we don't have libraries for unit test, maybe just lib/test/flags.go

If we use e2e it will conflict with the other one in test/e2e

If we use e2e it will conflict with the other one in test/e2e

How?

if we use test; the import path would look like (following Daisy's suggestion)

knative.dev/client/lib/test

while the one you mentioned would be

knative.dev/client/test/e2e

These are two different paths, I dont see any conflicts.
(even if we use e2e it would be lib/e2e/; still no conflicts as the parent path is different)

We need to get this right, as immediately after merge, there would be consumer of this import path.
Any subsequent change in this part of client repo would require changes in consumers as well.

daisy-ycguo · 2020-03-27T10:32:18Z

 */

-package e2e
+package integration


agree with e2e for short, other than integration. just feel too long.
If we don't have libraries for unit test, maybe just lib/test/flags.go

maximilien · 2020-03-27T16:28:17Z

Thanks for all the great feedback all. Will address today and push soon. Will get to it after lunch Pacific today.

maximilien · 2020-03-27T17:03:58Z

All feedback addressed. Let's see if we go green...

maximilien · 2020-03-27T17:09:50Z

/test pull-knative-client-build-tests

maximilien · 2020-03-27T18:16:54Z

/test pull-knative-client-build-tests

maximilien · 2020-03-28T00:21:13Z

/test pull-knative-client-build-tests

maximilien · 2020-03-28T00:37:11Z

OK foks we are green but pull-knative-client-build-tests and the error seems to be a flake:

go: extracting knative.dev/test-infra v0.0.0-20200327223851-0258d48ef3a8
go: finding github.com/google/licenseclassifier latest
go: finding github.com/sergi/go-diff v1.0.0
/home/prow/go/src/knative.dev/client
2020/03/28 00:24:27 No errors found.
Step failed: default_build_test_runner
============================
==== BUILD TESTS FAILED ====
============================
==== Fri Mar 27 17:24:27 PDT 2020
============================
Step failed: run_build_tests
+ EXIT_VALUE=1
+ set +o xtrace

maximilien · 2020-03-31T00:25:50Z

But I solved the problems by running custom go test with "tekton" build tag:

go test ./test/e2e/ -test.v -tags "tekton" -run TestTektonPipeline

I could see the issues you listed and solved them. Let's see where CI is with this latest.

Thanks @navidshaikh

maximilien · 2020-03-31T01:14:38Z

Yeah, all green @navidshaikh and @rhuss

Can I get a LGTM. Thanks.

cc: @daisy-ycguo

navidshaikh · 2020-03-31T05:59:02Z

 */

-package e2e
+package integration


If we use e2e it will conflict with the other one in test/e2e

How?

if we use test; the import path would look like (following Daisy's suggestion)

knative.dev/client/lib/test

while the one you mentioned would be

knative.dev/client/test/e2e

These are two different paths, I dont see any conflicts.
(even if we use e2e it would be lib/e2e/; still no conflicts as the parent path is different)

We need to get this right, as immediately after merge, there would be consumer of this import path.
Any subsequent change in this part of client repo would require changes in consumers as well.

maximilien · 2020-03-31T14:04:36Z

Ok I’ll change integration to test today and update the PR. Lots of meetings for me on Tuesdays but will find time to do this and push it today.

maximilien · 2020-03-31T17:17:53Z

@navidshaikh updated s/integration/test/, ready for LGTM. Thx.

navidshaikh

/lint

looking at result of refactoring, every lib util now need to take 3 testing related params, before it actually takes any operation related param, e.g:

func serviceCreate(t *testing.T, it *test.KnTest, r *test.KnRunResultCollector, serviceName string) {

There is repetition,
see:

type KnRunResultCollector struct {
  results    []KnRunResult
  extraDumps []string
  t          *testing.T
}

We can access testing.T from KnRunResultCollector in every test util function, this can be fixed
separately from this PR. @rhuss: WDYT?

knative-prow-robot

@navidshaikh: 5 warnings.

Details

In response to this:

/lint

looking at result of refactoring, every lib util now need to take 3 testing related params, before it actually takes any operation related param, e.g:
func serviceCreate(t *testing.T, it *test.KnTest, r *test.KnRunResultCollector, serviceName string) {
There is repetition,
see:
type KnRunResultCollector struct {
 results    []KnRunResult
 extraDumps []string
 t          *testing.T
}
We can access testing.T from KnRunResultCollector in every test util function, this can be fixed
separately from this PR. @rhuss: WDYT?

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.

navidshaikh

/lgtm

@maximilien : Lets merge this to unblock the -contrib work.
As a follow up to this PR, we should fix the docstring/typos.

navidshaikh · 2020-04-01T16:22:48Z

followup issue to be fixed: #769

maximilien · 2020-04-01T19:59:03Z

OK will push fix for #769 soon. Thanks

maximilien · 2020-04-01T20:36:56Z

KnRunResultCollector

OK makes sense. But let me send this as a separate PR since that changes signatures and might require more agreements. Whereas the docs changes are non-controversial.

…native#765) * (refactor) address the e2e extract / refactor of issue knative#763 * various updates to address reviewers feedback * renamed lib/test/integration to lib/test and package to test Signed-off-by: Roland Huß <roland@ro14nd.de> # Conflicts: # CHANGELOG.adoc # test/e2e/service_export_import_apply_test.go # test/e2e/trigger_test.go

* (refactor) address the e2e extract / refactor of issue #763 (#765) * (refactor) address the e2e extract / refactor of issue #763 * various updates to address reviewers feedback * renamed lib/test/integration to lib/test and package to test Signed-off-by: Roland Huß <roland@ro14nd.de> # Conflicts: # CHANGELOG.adoc # test/e2e/service_export_import_apply_test.go # test/e2e/trigger_test.go * fix(plugin): Fix plugin lookup with file ext on Windows (#774) * fix(plugin): Fix plugin lookup with file ext on Windows * chore: Update changelog * fix: Reflect review feedback * fix: Reflect review feedback and add future todo Signed-off-by: Roland Huß <roland@ro14nd.de> # Conflicts: # CHANGELOG.adoc * fix(issue #762): correct error message when updating service (#778) * fix(issue #762): correct error message when updating service * correct message when updating service and passing many names * fix issue with TestServiceUpdateWithMultipleImages running create vs update * * added TestServiceDescribeWithMultipleNames * added TestServiceCreateWithMultipleNames * fix error message for service delete since many names can be passed * Use vendored deps while running e2e locally (#783) Also set GO111MODULE=on unconditionally * Update sink binding create usage string (#785) * Add "--target-utilization" to manage "autoscaling.knative.dev/targetUtilizationPercentage" annotation (#788) * Support setting "autoscaling.knative.dev/targetUtilizationPercentage" annotation. Signed-off-by: Roland Huß <roland@ro14nd.de> # Conflicts: # test/e2e/service_options_test.go * Remove the delete propagation flag (#770) * Remove the delete propagation flag In it's current state it now takes me about 25 seconds for the `kn delete` to complete. Before #682 it used to be almost immediate. This is because we now pass in the `DeletePropagationBackground` flag. I believe this is a mistake, not only because of the 20+ seconds of additional time to delete things, but IMO the CLI should talk to the server in the same way regardless of the --wait flag. That flag should just be a CLI thing to indicate if the user wants the CLI to wait for the server to complete but not HOW the server should do the delete. Signed-off-by: Doug Davis <dug@us.ibm.com> * try just tweaking the --no-wait flag Signed-off-by: Doug Davis <dug@us.ibm.com> * Fix error when output is set to name (#775) * fix error when output is set to name * add e2e test * change to flags/listprint.go Signed-off-by: Roland Huß <roland@ro14nd.de> # Conflicts: # test/e2e/basic_workflow_test.go * Show all revisions when run `service describe -v` (#790) * The `kn service describe -v` command shows repetitive revisions, because the revision would be covered by next one. * Fix resource listing with -oname flag (#799) * Fix resource listing with -oname flag * add e2e tests Signed-off-by: Roland Huß <roland@ro14nd.de> # Conflicts: # test/e2e/ping_test.go # test/e2e/revision_test.go # test/e2e/route_test.go # test/e2e/source_apiserver_test.go # test/e2e/source_binding_test.go # test/e2e/trigger_test.go * Make wait, no-wait and async flags per bool var CLI convention (#802) * Make wait, no-wait and async flags per bool var CLI convention Fixes #800 - Deprecated bool vars can be supported for CLI convention - Bind --async flag value to --no-wait - Only one flag among [wait, no-wait, async] can be provided, else raise an error * Simplify conditionals * Add unit tests for deprecated flag async * Fix a typo * e2e: Foreground delete for revisions and services in e2e (#794) * e2e: Foreground delete for revisions and services in e2e to avoid any race conditions and flakes * Use --wait instead of --no-wait=false Signed-off-by: Roland Huß <roland@ro14nd.de> # Conflicts: # test/e2e/basic_workflow_test.go # test/e2e/revision_test.go * e2e: Run tekton e2e against pipeline v0.11.1 (#803) * Use buildah task from master branch and paramterize FORMAT * Configure pipeline v0.11.1 * DNM: Run tekton e2e in this PR * Revert "DNM: Run tekton e2e in this PR" This reverts commit 903f5be. * Update CHANGELOG for v0.13.2 (#804) * Pin serving to v0.13.2 and update version command (#797) * Pin serving v0.13.2 dep to v0.13.2 * Update version command now points to serving v0.13.2 and eventing v0.13.6 * Copy go.sum as generated in CI Signed-off-by: Roland Huß <roland@ro14nd.de> # Conflicts: # go.mod # go.sum # vendor/modules.txt * add missing vendored files * fixed error reporting for traffics tests * Updated test * fix formatting * e2e for service export (#739) * e2e for service export * e2e for service export * e2e for service export * e2e for service export * e2e for service export Signed-off-by: Roland Huß <roland@ro14nd.de> # Conflicts: # test/e2e/service_export_import_apply_test.go Co-authored-by: dr.max <maxim@us.ibm.com> Co-authored-by: David Simansky <dsimansk@redhat.com> Co-authored-by: Navid Shaikh <nshaikh@redhat.com> Co-authored-by: Lv Jiawei <lvjiawei@cmss.chinamobile.com> Co-authored-by: Doug Davis <dug@us.ibm.com> Co-authored-by: Ying Chun Guo <guoyingc@cn.ibm.com> Co-authored-by: Murugappan Chetty <itsmurugappan@gmail.com>

…signature-scan:0.2 Docker digest to e127e0e (knative#765) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | quay.io/konflux-ci/konflux-vanguard/task-rpms-signature-scan | tekton-bundle | digest | `b5c2ba8` -> `e127e0e` | --- ### Configuration 📅 **Schedule**: Branch creation - "after 5am on saturday" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- To execute skipped test pipelines write comment `/ok-to-test`. This PR has been generated by [MintMaker](https://redirect.github.com/konflux-ci/mintmaker) (powered by [Renovate Bot](https://redirect.github.com/renovatebot/renovate)).  Signed-off-by: red-hat-konflux-kflux-prd-rh02 <190377777+red-hat-konflux-kflux-prd-rh02[bot]@users.noreply.github.com> Co-authored-by: red-hat-konflux-kflux-prd-rh02[bot] <190377777+red-hat-konflux-kflux-prd-rh02[bot]@users.noreply.github.com>

(refactor) address the e2e extract / refactor of issue knative#763

df3334c

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 27, 2020

knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 27, 2020

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

knative-prow-robot requested review from cppforlife and mattmoor March 27, 2020 03:11

Merge branch 'master' into issue763_e2e_refactor

322850c

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 27, 2020

mattmoor reviewed Mar 27, 2020

View reviewed changes

maximilien requested a review from mattmoor March 27, 2020 03:22

knative-prow-robot assigned daisy-ycguo and rhuss Mar 27, 2020

navidshaikh suggested changes Mar 27, 2020

View reviewed changes

daisy-ycguo suggested changes Mar 27, 2020

View reviewed changes

maximilien force-pushed the issue763_e2e_refactor branch from 1019ab3 to fd91fc7 Compare March 27, 2020 17:03

maximilien force-pushed the issue763_e2e_refactor branch from fd91fc7 to 10e07b3 Compare March 27, 2020 18:08

maximilien force-pushed the issue763_e2e_refactor branch 2 times, most recently from fa9b599 to 37aa3e1 Compare March 27, 2020 22:12

various updates to address reviewers feedback

224dfe8

maximilien force-pushed the issue763_e2e_refactor branch from 854e2d3 to 224dfe8 Compare March 31, 2020 00:28

maximilien mentioned this pull request Mar 31, 2020

Extract common kn functionality for plugins and specialized Knative clients #763

Closed

4 tasks

navidshaikh suggested changes Mar 31, 2020

View reviewed changes

renamed lib/test/integration to lib/test and package to test

9ca020b

navidshaikh reviewed Apr 1, 2020

View reviewed changes

Comment thread lib/test/integration.go

Comment thread CHANGELOG.adoc

Comment thread lib/test/integration.go

Comment thread lib/test/cli.go

knative-prow-robot reviewed Apr 1, 2020

View reviewed changes

Comment thread test/e2e/service_options_test.go

Comment thread test/e2e/traffic_split_test.go

Comment thread lib/test/result_collector.go

Comment thread test/e2e/source_apiserver_test.go

Comment thread test/e2e/source_apiserver_test.go

navidshaikh reviewed Apr 1, 2020

View reviewed changes

knative-prow-robot assigned navidshaikh Apr 1, 2020

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

knative-prow-robot merged commit df78b8b into knative:master Apr 1, 2020

navidshaikh mentioned this pull request Apr 1, 2020

test: fix typos/docstrings #769

Closed

maximilien deleted the issue763_e2e_refactor branch April 1, 2020 20:00

maximilien mentioned this pull request Apr 1, 2020

refactor: simplify parameters in e2e tests #772

Closed

navidshaikh added the backport/candidate Consider this PR to be backported to the release branch label Apr 15, 2020

rhuss mentioned this pull request Apr 15, 2020

Pr/release 0.13.2 backports #806

Merged

navidshaikh added backport/pr A backport PR which is target to a release branch. and removed backport/candidate Consider this PR to be backported to the release branch labels Apr 20, 2020

rhuss added backported-to/0.13 and removed backport/pr A backport PR which is target to a release branch. labels Apr 20, 2020

Conversation

maximilien commented Mar 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Reference

Uh oh!

knative-prow-robot commented Mar 27, 2020

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

maximilien commented Mar 27, 2020

Uh oh!

maximilien commented Mar 27, 2020

Uh oh!

navidshaikh commented Mar 27, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

navidshaikh Mar 27, 2020

Choose a reason for hiding this comment

Uh oh!

daisy-ycguo Mar 27, 2020

Choose a reason for hiding this comment

Uh oh!

maximilien Mar 27, 2020

Choose a reason for hiding this comment

Uh oh!

navidshaikh Mar 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

daisy-ycguo Mar 27, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maximilien commented Mar 27, 2020

Uh oh!

maximilien commented Mar 27, 2020

Uh oh!

maximilien commented Mar 27, 2020

Uh oh!

maximilien commented Mar 27, 2020

Uh oh!

maximilien commented Mar 28, 2020

Uh oh!

maximilien commented Mar 28, 2020

Uh oh!

maximilien commented Mar 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maximilien commented Mar 31, 2020

Uh oh!

navidshaikh Mar 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maximilien commented Mar 31, 2020

Uh oh!

maximilien commented Mar 31, 2020

Uh oh!

navidshaikh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maximilien commented Mar 27, 2020 •

edited

Loading

navidshaikh Mar 31, 2020 •

edited

Loading

maximilien commented Mar 31, 2020 •

edited

Loading

navidshaikh Mar 31, 2020 •

edited

Loading

maximilien commented Apr 1, 2020 •

edited

Loading