-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Update test/adding_tests.md #6345
Conversation
Hi @MIBc. 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 Once the patch is verified, the new status will be reflected by the 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. |
/assign @adrcunha |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating the docs, just a few minor things.
/ok-to-test
test/adding_tests.md
Outdated
@@ -49,24 +49,31 @@ See [`knative/pkg/test`](https://github.com/knative/pkg/tree/master/test) to: | |||
These flags are useful for running against an existing cluster, making use of | |||
your existing [environment setup](../DEVELOPMENT.md#setup-your-environment). | |||
|
|||
By importing `github.com/knative/serving/test` you get access to a global | |||
By importing `knative.dev/pgk/test` you get access to a global |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By importing `knative.dev/pgk/test` you get access to a global | |
By importing `knative.dev/pkg/test` you get access to a global |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
- [`--tag`](#using-a-docker-tag) | ||
- [`--ingressendpoint`](#using-a-custom-ingress-endpoint) | ||
- [All flags added by `knative/pkg/test`](https://github.com/knative/pkg/tree/master/test#flags) such as: | ||
- [`--dockerrepo`](#overriding-docker-repo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these be on the same indentation level as the last three flags?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think knative/pkg/test
flag and knative/serving/test
flag can be same level.
The --dockerrepo
, --tag
, --ingressendpoint
belong to knative/pkg/test
. They are subset of knative/pkg/test
flag.
The last three flags are special flag for knative/serving/test
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah got it. Thanks for the clarification!
- `Routes` | ||
- `Configurations` | ||
- `Revisions` | ||
- `Knative ingress` | ||
- `ServerlessServices` | ||
- `Istio objects` | ||
|
||
For example, to create a `Route`: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line needs to be updated:
_, err = clients.ServingClient.Routes.Create(test.Route(namespaceName, routeName, configName))
-->
_, err = clients.ServingClient.Routes.Create(v1test.Route(
test.ResourceNames{
Route: routeName,
Config: configName,
}))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
test/adding_tests.md
Outdated
|
||
```go | ||
err = test.WaitForRouteState(clients.ServingClient, routeName, test.AllRouteTrafficAtRevision(routeName, revisionName)) | ||
err := v1alpha1testing.WaitForRouteState(clients.ServingClient, routeName, v1alpha1testing.AllRouteTrafficAtRevision(routeName, revisionName)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be updated because AllRouteTrafficAtRevision
takes type ResourceNames
as the argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
test/adding_tests.md
Outdated
|
||
```go | ||
err := test.WaitForRevisionState(clients.ServingClient, revisionName, test.IsRevisionReady(revisionName)) | ||
err := v1alpha1testing.WaitForRevisionState(clients.ServingClient, revisionName, v1alpha1testing.IsRevisionReady(revisionName)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be updated because IsRevisionReady
takes the revision object as the argument, not the name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The WaitForRevisionState also need to be updated.
Update test/adding_tests.md and test/README.md Fixes knative#6239
Do you have any other comments? @adrcunha |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adrcunha, MIBc 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 |
/lgtm |
Update test/adding_tests.md and test/README.md
Fixes #6239
Proposed Changes