Support multiple arguments on revision delete#657
Support multiple arguments on revision delete#657knative-prow-robot merged 13 commits intoknative:masterfrom
Conversation
knative-prow-robot
added
the
size/M
navidshaikh
left a comment
navidshaikh
left a comment
There was a problem hiding this comment.
/retest
thanks!
just a few minor changes requested
| existRevision1 := revisionNames[0] | ||
| existRevision2 := revisionNames[1] | ||
| nonexistRevision := revisionNames[2] | ||
| out, err := test.kn.RunWithOpts([]string{"revision", "list"}, runOpts{NoNamespace: false}) | ||
| assert.NilError(t, err) | ||
| assert.Check(t, strings.Contains(out, existRevision1), "Required revision1 does not exist") | ||
| assert.Check(t, strings.Contains(out, existRevision2), "Required revision2 does not exist") | ||
| assert.Check(t, !strings.Contains(out, nonexistRevision), "Nonexistent revision does exist") |
There was a problem hiding this comment.
I think this block is not required as if the revision doesnt exist, it should be tested in delete check below.
There was a problem hiding this comment.
I'd rather keep it as if the tests before this one are moved around there might not be 2 revisions. Though I'll remove it if you feel strongly.
There was a problem hiding this comment.
In that case, your tests should generate revisions by its own and not rely on generating test artifacts by other tests.
|
/retest The test should be more stable when #661 is merged. |
|
navidshaikh
left a comment
navidshaikh
left a comment
There was a problem hiding this comment.
--- FAIL: TestRevision/delete_three_revisions_with_one_revision_a_nonexistent (15.36s)
revision_test.go:111: assertion failed:
Actual output: Revision 'hello-zxgvs-1' deleted in namespace 'kne2etests0'.
Revision 'hello-vvpnh-2' deleted in namespace 'kne2etests0'.
revisions.serving.knative.dev "hello-nonexist" not found.
Missing strings: successfully: Failed to get 'successfully deleted' first revision message
revision_test.go:112: assertion failed:
Actual output: Revision 'hello-zxgvs-1' deleted in namespace 'kne2etests0'.
Revision 'hello-vvpnh-2' deleted in namespace 'kne2etests0'.
revisions.serving.knative.dev "hello-nonexist" not found.
Missing strings: successfully: Failed to get 'successfully deleted' second revision message
looks like tests need to be updated with change in messages
|
fixed the tests, and actually checked the tests TestRevisions before pushing :) |
|
The following is the coverage report on the affected files.
|
|
/retest |
knative-prow-robot
added
the
approved
navidshaikh
left a comment
navidshaikh
left a comment
There was a problem hiding this comment.
/lgtm
/approve
Thanks!
| revName1 := "foo-12345" | ||
| revName2 := "foo-67890" | ||
| revName3 := "foo-abcde" | ||
| action, _, output, err := fakeRevisionDelete([]string{"revision", "delete", revName1, revName2, revName3}) |
There was a problem hiding this comment.
Take a look at this unit tests framework we're using now https://github.com/knative/client/blob/master/pkg/kn/commands/service/service_update_mock_test.go#L32
We should eventually streamline all the unit tests to use single framework.
| } | ||
|
|
||
| func (test *e2eTest) revisionDelete(t *testing.T, serviceName string) { | ||
| revName := test.findRevision(t, serviceName) |
|
|
||
| t.Run("delete latest revision from hello service and return no error", func(t *testing.T) { | ||
| test.revisionDelete(t, "hello") | ||
| revName := test.findRevision(t, "hello") |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: maximilien, navidshaikh, wslyln The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
7 participants
Addresses #317
Release note:
kn revision delete