Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Status conformance tests for channels #3000

Merged

Conversation

aliok
Copy link
Member

@aliok aliok commented Apr 16, 2020

Fixes #2992

Proposed Changes

Verification happy case:

go test -v -timeout 30s -tags e2e knative.dev/eventing/test/conformance -run ^TestChannelStatus$ -channels="messaging.knative.dev/v1alpha1:InMemoryChannel,messaging.knative.dev/v1alpha1:Channel,messaging.knative.dev/v1beta1:InMemoryChannel"

Verification unhappy case:
To make the test fail for a non-compliant channel:

  • create a CRD like following
cat <<EOS |kubectl apply -f -
---
apiVersion: apiextensions.k8s.io/v1beta1
kind: CustomResourceDefinition
metadata:
  name: foochannels.example.com
spec:
  group: example.com
  versions:
    - name: v1
      served: true
      storage: true
  scope: Namespaced
  names:
    plural: foochannels
    singular: foochannel
    kind: FooChannel
EOS
  • comment out the client.WaitForResourceReadyOrFail(channelName, &channel) line as the CR for that CRD won't be Ready never ever
  • Run the test:
go test -v -timeout 30s -tags e2e knative.dev/eventing/test/conformance -run ^TestChannelStatus$ -channels="example.com/v1:FooChannel"

The test will fail:

{"FooChannel" "example.com/v1"} does not have status.address

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Apr 16, 2020
@knative-prow-robot knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/test-and-release Test infrastructure, tests or release labels Apr 16, 2020
@matzew
Copy link
Member

matzew commented Apr 16, 2020

/test pull-knative-eventing-unit-tests

@aliok
Copy link
Member Author

aliok commented Apr 16, 2020

@aslom @matzew mind reviewing?

@aliok
Copy link
Member Author

aliok commented Apr 16, 2020

I will write a separate test for the negative case and send a new PR.
(thanks @matzew for the suggestion)

@matzew
Copy link
Member

matzew commented Apr 16, 2020

/lgtm

leaving final review/approval for @aslom

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 16, 2020
@aslom
Copy link
Member

aslom commented Apr 16, 2020

Great work

/lgtm

@matzew
Copy link
Member

matzew commented Apr 16, 2020

/approve

@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aliok, matzew

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:

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

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 16, 2020
@knative-prow-robot knative-prow-robot merged commit 618199c into knative:master Apr 16, 2020
@aliok
Copy link
Member Author

aliok commented Apr 16, 2020

I will write a separate test for the negative case and send a new PR.

I won't do it.
This test is about conformance of the channels. So, if you have a new channel it should pass these conformance tests. Testing an imaginary channel against the conformance tests didn't sound good on a second thought.
If we really want to "test the test", we might come up with a separate execution of this test for the imaginary channel and see if the test fails.

@aliok aliok deleted the channel_status_conformance_tests branch April 17, 2020 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test-and-release Test infrastructure, tests or release cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Conformance tests for channel status requirements
5 participants