Add E2E tests for Service, Revision, Route by mgencur · Pull Request #291 · knative/client

mgencur · 2019-07-24T11:45:04Z

service, revision, route describe and describe with print flags
route list with print flags
service create for a duplicate service

Fixes #

Proposed Changes

increase test coverage via new E2E tests

Release Note

* service, revision, route describe and describe with print flags * route list with print flags * service create for a duplicate service

knative-prow-robot

@mgencur: 0 warnings.

Details

In response to this:

service, revision, route describe and describe with print flags

route list with print flags

service create for a duplicate service

Fixes #

Proposed Changes

increase test coverage via new E2E tests

Release Note

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 · 2019-07-24T11:59:31Z

+	assert.Check(t, strings.Contains(out, serviceName), "The service does not exist yet")
+
+	_, err = test.kn.RunWithOpts([]string{"service", "create",
+		fmt.Sprintf("%s", serviceName),


just use serviceName here ? No need for fmt.Sprintf

navidshaikh · 2019-07-24T12:03:59Z

+	assert.NilError(t, err)
+
+	expectedName := fmt.Sprintf("service.serving.knative.dev/%s", serviceName)
+	assert.Check(t, strings.Contains(out, expectedName))


this should be an exact string match IMO, i..e assert.Equal(t, out, expectedName)

as this is not string manipulation and printing on the client side, but machine readable output

navidshaikh · 2019-07-24T12:04:50Z

+	assert.NilError(t, err)
+
+	expectedName := fmt.Sprintf("route.serving.knative.dev/%s", routeName)
+	assert.Check(t, strings.Contains(out, expectedName))


same as above about exact name matching

navidshaikh · 2019-07-24T12:08:20Z

+	assert.Check(t, util.ContainsAll(out, serviceName))
+}
+
+func (test *e2eTest) serviceCreateDuplicate(t *testing.T, serviceName string) {


IMO, this particular test case doesn't belong to basic_workflow_test as the filename suggest, but kind of testing for error handling mechanism, we should probably create another workflow which specifically aims at error handling test cases.

If you all decide to move this to a more thorough IT (I am ambivalent at this point), please do the same for suggestion above on delete service.

should be addressed as part of resolution to #301

navidshaikh · 2019-07-24T12:12:47Z

+	assert.NilError(t, err)
+
+	expectedName := fmt.Sprintf("revision.serving.knative.dev/%s", revName)
+	assert.Check(t, strings.Contains(out, expectedName))


same comment as above, just check for exact match

maximilien · 2019-07-24T21:50:07Z

 	})

+	t.Run("create hello service again and get service already exists error", func(t *testing.T) {
+		test.serviceCreateDuplicate(t, "hello")


Nice nice, I wonder if we should also try to delete a service that’s already deleted? I would expect an error in case where service is already deleted (non-existent) and if the service is being deleted. Of course in the end the API should do the right thing and the client should return a “sensible” error.

Yeah. Good point. I can add a test for deleting a non-existent service to this PR.

maximilien · 2019-07-24T21:51:24Z

+	assert.Check(t, util.ContainsAll(out, serviceName))
+}
+
+func (test *e2eTest) serviceCreateDuplicate(t *testing.T, serviceName string) {


If you all decide to move this to a more thorough IT (I am ambivalent at this point), please do the same for suggestion above on delete service.

maximilien · 2019-07-24T21:55:13Z

/ok-to-test

maximilien · 2019-07-25T04:44:21Z

/retest

mgencur · 2019-07-25T08:07:17Z

hey! Thanks for the reviews. I have fixed all things except moving the test. I also added one more test that checks deleting a non-existent service.
Regarding moving the test from the basic workflow, I spoke with @navidshaikh and I will shortly create an issue with a proposal for restructuring the test suite a little bit. When we agree on what to do we can move the test(s) to appropriate locations.

mgencur · 2019-07-25T08:56:05Z

@navidshaikh the failure doesn't seem to be related to my changes. Can you advise what to do, pls?

rhuss · 2019-07-25T09:24:43Z

/retest

rhuss · 2019-07-25T09:26:14Z

I'm not sure about that flake, because I thought it was fixed with #271 . It might be that we have to increase the timeout for the wait (as if there's a timeout, then the previous update command returns prematurely).

I just triggered a retest. Let's see how it goes ...

mgencur · 2019-07-25T12:46:19Z

Passed this time:)

maximilien

/lgtm

maximilien · 2019-07-25T20:25:00Z

@mgencur not quite...

Running 'kubectl delete namespace kne2etests0'...
--- FAIL: TestBasicWorkflow (126.82s)
    --- PASS: TestBasicWorkflow/returns_no_service_before_running_tests (0.05s)
    --- PASS: TestBasicWorkflow/create_hello_service_and_returns_no_error (24.58s)
    --- PASS: TestBasicWorkflow/create_hello_service_again_and_get_service_already_exists_error (0.12s)
    --- PASS: TestBasicWorkflow/returns_valid_info_about_hello_service (0.14s)
    --- PASS: TestBasicWorkflow/update_hello_service's_configuration_and_returns_no_error (0.11s)
    --- PASS: TestBasicWorkflow/create_another_service_and_returns_no_error (2.99s)
    --- FAIL: TestBasicWorkflow/returns_a_list_of_revisions_associated_with_hello_and_svc2_services (0.13s)
        basic_workflow_test.go:132: assertion failed: 
            Actual output: hello-kxgvk   hello     3s    1 OK / 5     Unknown   Deploying
            Missing strings: True
...

Might be flaky and timing to allow status to update.

navidshaikh · 2019-07-26T05:50:59Z

+	out, err := test.kn.RunWithOpts([]string{"service", "describe", serviceName, "-o=name"}, runOpts{})
+	assert.NilError(t, err)
+
+	expectedName := fmt.Sprintf("service.serving.knative.dev/%s", serviceName)


this error message needs to be fixed on kn side, I will have a PR for this fix.

navidshaikh

lgtm

knative-prow-robot · 2019-07-26T05:51:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maximilien, mgencur, navidshaikh

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

navidshaikh · 2019-07-26T05:51:49Z

/test pull-knative-client-integration-tests-latest-release

Add E2E tests for Service, revision, route

2bcda3c

* service, revision, route describe and describe with print flags * route list with print flags * service create for a duplicate service

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

knative-prow-robot reviewed Jul 24, 2019

View reviewed changes

knative-prow-robot requested review from mattmoor and sixolet July 24, 2019 11:45

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 24, 2019

navidshaikh reviewed Jul 24, 2019

View reviewed changes

maximilien suggested changes Jul 24, 2019

View reviewed changes

knative-prow-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jul 24, 2019

Test serviceDeleteNonexistent and fixes

ed8fd40

maximilien approved these changes Jul 25, 2019

View reviewed changes

knative-prow-robot assigned maximilien Jul 25, 2019

knative-prow-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 25, 2019

navidshaikh reviewed Jul 26, 2019

View reviewed changes

navidshaikh approved these changes Jul 26, 2019

View reviewed changes

knative-prow-robot merged commit 03ecb36 into knative:master Jul 26, 2019

Conversation

mgencur commented Jul 24, 2019

Proposed Changes

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maximilien commented Jul 24, 2019

Uh oh!

maximilien commented Jul 25, 2019

Uh oh!

mgencur commented Jul 25, 2019

Uh oh!

mgencur commented Jul 25, 2019

Uh oh!

rhuss commented Jul 25, 2019

Uh oh!

rhuss commented Jul 25, 2019

Uh oh!

mgencur commented Jul 25, 2019

Uh oh!

maximilien left a comment

Choose a reason for hiding this comment

Uh oh!

maximilien commented Jul 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

navidshaikh left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Jul 26, 2019

Uh oh!

navidshaikh commented Jul 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants