Skip to content

Add PingSource conformance tests initialization#3680

Merged
knative-prow-robot merged 4 commits into
knative:masterfrom
devguyio:3117-ping-src-status-conf
Jul 27, 2020
Merged

Add PingSource conformance tests initialization#3680
knative-prow-robot merged 4 commits into
knative:masterfrom
devguyio:3117-ping-src-status-conf

Conversation

@devguyio
Copy link
Copy Markdown
Contributor

@devguyio devguyio commented Jul 22, 2020

Proposed Changes

  • Add PingSource conformance initialization code
  • Configure PingSource properly to be picked up by SourceStatus conformance tests
  • Explicitly skip the test in ComponentTestRunner.RunTestsWithComponentOptions when in strict mode and tests are not run

This PR is based on #3605

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

devguyio commented Jul 22, 2020

Holding until #3605 gets merged
/hold

@knative-prow-robot knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 22, 2020
antoineco
antoineco reviewed Jul 22, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@antoineco antoineco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job! The PR is massive but here are a few comments focused on the code quality.

Comment thread test/conformance/helpers/sources/source_status_test_helper.go Outdated
Comment thread test/conformance/helpers/sources/source_crd_metadata_test_helper.go
Comment thread test/conformance/helpers/sources/source_status_test_helper.go
Comment thread test/conformance/helpers/sources/source_status_test_helper.go
Comment thread test/conformance/helpers/sources/source_status_test_helper.go Outdated
Comment thread test/conformance/main_test.go
Comment thread test/conformance/main_test.go Outdated
Comment thread test/lib/setupclientoptions/sources.go
Comment thread test/lib/test_runner.go
Comment thread test/lib/test_runner.go Outdated
@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 24, 2020
@aliok
Copy link
Copy Markdown
Member

aliok commented Jul 27, 2020

/assign

@devguyio devguyio force-pushed the 3117-ping-src-status-conf branch from 490ba20 to 1ef6b74 Compare July 27, 2020 08:05
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 27, 2020
@knative-test-reporter-robot
Copy link
Copy Markdown

knative-test-reporter-robot commented Jul 27, 2020

The following jobs failed:

Test name Triggers Retries
pull-knative-eventing-integration-tests 0/3

Failed non-flaky tests preventing automatic retry of pull-knative-eventing-integration-tests:

test/e2e.TestSingleStructuredEventForChannelV1Beta1
test/e2e.TestSingleStructuredEventForChannelV1Beta1/InMemoryChannel-messaging.knative.dev/v1

@devguyio
Copy link
Copy Markdown
Contributor Author

devguyio commented Jul 27, 2020

/uncc @yt3liu
/uncc @steuhs

@devguyio
Copy link
Copy Markdown
Contributor Author

devguyio commented Jul 27, 2020

@antoineco I updated the formatting and indeed enabled go fmt on save. Thanks for the review and the tip

@antoineco
Copy link
Copy Markdown
Contributor

antoineco commented Jul 27, 2020 •
edited
Loading

@devguyio the "9 hidden conversations" are unaddressed, GitHub has a bad tendency to hide half of the review comments :) Some of them are just nits and can be omitted, but I think the ones I bumped are important.

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 27, 2020
@devguyio devguyio force-pushed the 3117-ping-src-status-conf branch from 3b4eb03 to ec80699 Compare July 27, 2020 09:59
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 27, 2020
@devguyio
Copy link
Copy Markdown
Contributor Author

devguyio commented Jul 27, 2020

@antoineco ah yeah! thanks! done.

antoineco
antoineco suggested changes Jul 27, 2020
View reviewed changes
Comment thread test/conformance/helpers/sources/source_status_test_helper.go Outdated
@antoineco
Copy link
Copy Markdown
Contributor

antoineco commented Jul 27, 2020 •
edited
Loading

Just for my own curiosity, why splitting so many lines in the middle? I haven't managed to understand the pattern you had in mind. (see hidden review comments)

@devguyio
Copy link
Copy Markdown
Contributor Author

devguyio commented Jul 27, 2020

@antoineco 80 columns limit. I replied to all those comments that I try to respect 80 columns limit 🙂

@antoineco
Copy link
Copy Markdown
Contributor

antoineco commented Jul 27, 2020

Most of the linters are configured to ignore lines shorter than 120 chars nowadays. The Knative code uses that rule in most places.

@devguyio @antoineco
Add a clearer log message
c575715 
Co-authored-by: Antoine Cotten <hello@acotten.com>
@antoineco
Copy link
Copy Markdown
Contributor

antoineco commented Jul 27, 2020

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 27, 2020
@devguyio
Copy link
Copy Markdown
Contributor Author

devguyio commented Jul 27, 2020 •
edited
Loading

@antoineco I am aware of the linters default 120 and I understand it's a controversial topic whether to do 80 or 120, you'll see a lot of reasons why to use 80 that are still relevant today. I guess I'm one of the 80s guys. As for Knative, I failed to find a "unified style guide" for Knative hence I stick to what I find logical until as a community we make up our mind.

@slinkydeveloper
Copy link
Copy Markdown
Contributor

slinkydeveloper commented Jul 27, 2020

/lgtm
/approve

@knative-prow-robot
Copy link
Copy Markdown
Contributor

knative-prow-robot commented Jul 27, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: antoineco, devguyio, slinkydeveloper

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:

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 Jul 27, 2020
@devguyio
Copy link
Copy Markdown
Contributor Author

devguyio commented Jul 27, 2020

/unhold

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 27, 2020
@knative-prow-robot knative-prow-robot merged commit 54fdd6c into knative:master Jul 27, 2020
@devguyio devguyio mentioned this pull request Jul 28, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@antoineco antoineco antoineco approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@aliok aliok

@antoineco antoineco

@slinkydeveloper slinkydeveloper

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@devguyio @aliok @knative-test-reporter-robot @antoineco @slinkydeveloper @knative-prow-robot @googlebot