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.

Sign up for GitHub

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

Jump to bottom

Add readme for tests (#554) #555

Merged
merged 23 commits into from
Dec 17, 2019
Merged

Add readme for tests (#554) #555

merged 23 commits into from
Dec 17, 2019

Conversation

itsmurugappan
Copy link
Contributor

Fixes #554

Proposed Changes

  • Add Readme file for tests
  • Steps include, how to unit test and e2e locally

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Dec 15, 2019
@knative-prow-robot
Copy link
Contributor

Hi @itsmurugappan. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@knative-prow-robot knative-prow-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 15, 2019
navidshaikh
navidshaikh requested changes Dec 16, 2019
View reviewed changes
Copy link
Collaborator

@navidshaikh navidshaikh left a comment

Choose a reason for hiding this comment

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

Thanks for adding the docs!

Suggested few changes mostly around re-using test/e2e-tests-local.sh script if user wants to pass
additional args to go test. Another for referencing the e2e-tests-local.sh with absolute path from repo root (i.e. test/e2e-tests-local.sh)

test/README.md Outdated Show resolved Hide resolved
test/README.md Outdated Show resolved Hide resolved
test/README.md Outdated Show resolved Hide resolved
test/README.md Outdated Show resolved Hide resolved
test/README.md Outdated Show resolved Hide resolved
test/README.md Outdated Show resolved Hide resolved
test/README.md Outdated Show resolved Hide resolved
test/README.md Outdated Show resolved Hide resolved
test/README.md Outdated Show resolved Hide resolved
test/README.md Outdated Show resolved Hide resolved
itsmurugappan and others added 15 commits December 16, 2019 10:08
@itsmurugappan @navidshaikh
Update test/README.md
83cb603 
Co-Authored-By: Navid Shaikh <[email protected]>
@itsmurugappan @navidshaikh
Update test/README.md
eee7f0d 
Co-Authored-By: Navid Shaikh <[email protected]>
@itsmurugappan @navidshaikh
Update test/README.md
6757603 
Co-Authored-By: Navid Shaikh <[email protected]>
@itsmurugappan @navidshaikh
Update test/README.md
fec9026 
Co-Authored-By: Navid Shaikh <[email protected]>
@itsmurugappan @navidshaikh
Update test/README.md
cfc8b0a 
Co-Authored-By: Navid Shaikh <[email protected]>
@itsmurugappan @navidshaikh
Update test/README.md
da0379b 
Co-Authored-By: Navid Shaikh <[email protected]>
@itsmurugappan @navidshaikh
Update test/README.md
9351795 
Co-Authored-By: Navid Shaikh <[email protected]>
@itsmurugappan @navidshaikh
Update test/README.md
e55b5c6 
Co-Authored-By: Navid Shaikh <[email protected]>
@itsmurugappan @navidshaikh
Update test/README.md
cf2f454 
Co-Authored-By: Navid Shaikh <[email protected]>
@itsmurugappan @navidshaikh
Update test/README.md
a67f266 
Co-Authored-By: Navid Shaikh <[email protected]>
@itsmurugappan @navidshaikh
Update test/README.md
181f24e 
Co-Authored-By: Navid Shaikh <[email protected]>
@itsmurugappan @navidshaikh
Update test/README.md
4667fbc 
Co-Authored-By: Navid Shaikh <[email protected]>
@itsmurugappan
review changes for pr #555
a047fc3
@itsmurugappan
review changes for pr #555
90345c8
@itsmurugappan
review changes for pr #555
58be5a0
maximilien
maximilien approved these changes Dec 16, 2019
View reviewed changes
Copy link
Contributor

@maximilien maximilien left a comment

Choose a reason for hiding this comment

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

Please also add a link from the main README.md to this one.

Thanks for your contribution.

test/README.md Show resolved Hide resolved
@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 16, 2019
@rhuss
Copy link
Contributor

rhuss commented Dec 16, 2019

/ok-to-test

@knative-prow-robot knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 16, 2019
@itsmurugappan
Copy link
Contributor Author

itsmurugappan commented Dec 16, 2019 •
edited
Loading

Please also add a link from the main README.md to this one.

Thanks for your contribution.

Added to DEVELOPMENT.md file. is that fine ?

navidshaikh
navidshaikh reviewed Dec 17, 2019
View reviewed changes
Copy link
Collaborator

@navidshaikh navidshaikh left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

Few more suggestion, would do that by another PR.
Thanks!

test/README.md
To run one e2e test case, e.g. TestBasicWorkflow

```bash
go test -v -tags=e2e -count=1 ./e2e -run ^TestBasicWorkflow$
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can do this as suggested test/e2e-tests-local.sh -run ^TestBasicWorkflow$

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@navidshaikh thank you.
Instead of 'go test' use the script ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, see #555 (comment)

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 17, 2019
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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:
  • OWNERS [maximilien,navidshaikh]

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 merged commit 8a9d8fc into knative:master Dec 17, 2019
@itsmurugappan itsmurugappan mentioned this pull request Dec 20, 2019
Merged
@itsmurugappan itsmurugappan mentioned this pull request Jan 8, 2020
Merged
knative-prow-robot pushed a commit that referenced this pull request Jan 20, 2020
@itsmurugappan @knative-prow-robot
Fix misleading error message when verb is bad (#589)
de7e388 
* validate sub command verb (#557)

* fix review comments for pr #555

* validate sub commands

* update error message

* use inner args returned by find func

* return appropriate error message for #557

* return appropriate error message for #557

* Revert "fix review comments for pr #555"

This reverts commit 14ea8d2.

* review comments for pr 589

* misleading error message when verb is bad (#557)

* misleading error message when verb is bad (#557)

* misleading error message when verb is bad (#557)
mgencur referenced this pull request in openshift-knative/client Nov 21, 2022
@navidshaikh
[0.18.x] Update SPEC file to pick the latest go version (#555)
6f593b9 
go 1.14.12
dsimansk added a commit to dsimansk/client that referenced this pull request Jan 8, 2025
@matejvasek @dsimansk
[release-v1.15] Bump func (knative#555)
37c9524 
Signed-off-by: Matej Vašek <[email protected]>
Co-authored-by: David Simansky <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@navidshaikh navidshaikh navidshaikh requested changes

@maximilien maximilien maximilien approved these changes

@mattmoor mattmoor Awaiting requested review from mattmoor

Assignees

@navidshaikh navidshaikh

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Readme for tests
6 participants
@itsmurugappan @knative-prow-robot @rhuss @maximilien @navidshaikh @googlebot