-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
📖 doc: add local e2e test execution documentation #4626
📖 doc: add local e2e test execution documentation #4626
Conversation
cff6324
to
f734f53
Compare
I'm not sure if it's the right place in the book, but I thought I just start somewhere. |
@sbueringer: GitHub didn't allow me to request PR reviews from the following users: stmcginnis. Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
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. |
Thanks for getting something written up Stefan! |
f734f53
to
6a8d634
Compare
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.
/lgtm
tested locally, this is great! |
6a8d634
to
b2fd889
Compare
@stmcginnis @CecileRobertMichon @fabriziopandini I merged the new documentation with the pre-existing in @fabriziopandini Regarding #4626 (comment): I kept the few sentences about Prow for now. I would replace them by a link to the dedicated Prow page after it has been added. |
/retest |
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.
/lgtm
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 generally looks good to me, i have a few minor suggestions.
For development and debugging those tests can also be executed locally. | ||
|
||
### Prerequisites | ||
|
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.
does the user need anything beyond docker and the make/build commands?
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 it depends on which exact test is executed and in which way it is executed. Afaik:
- make
- requires at least kustomize and kind (ci-e2e.sh installs them via hack/ensure-*.sh)
- additionally via IDE / scripts/ci-e2e.sh
- when new kindest/node images have to be build (i.e. they don't exist remotely) it builds the images locally and depends on the existence of "$GOPATH/src/k8s.io/kubernetes" (This should be mostly solved by Generate images for CAPD machines to be used in our E2E testing #4620)
Looking at ci-e2e.sh
we make sure to have the following installed: go, kubectl, kustomize, kind. There's also a lot of other stuff installed here: https://github.com/kubernetes/test-infra/blob/master/images/bootstrap/Dockerfile. But I don't think we need anything additionally to run the tests locally.
I can document this on a best effort basis. Maybe @fabriziopandini knows some additional dependencies.
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.
Let's start with what we have now, and then we can eventually iterate adding more context.
However, generally speaking we should document in single place the dependencies for a developer workstation, and reference this list whenever necessary, like in this page.
As of today we already have something similar in two places, which is not ideal
- https://master.cluster-api.sigs.k8s.io/developer/guide.html
- https://master.cluster-api.sigs.k8s.io/developer/tilt.html
So I think we should defer fixing this to a follow up PR
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 it depends on which exact test is executed and in which way it is executed.
@sbueringer, i was just thinking about the bare minimum, not necessarily all the options
As of today we already have something similar in two places, which is not ideal
++, would be nice to have a single place that we could link
/retest |
@sbueringer if you can squash commits, I'm +1 to get this merged given that it is a great step forward vs what we have now |
This is great. I hate having the hidden variables in the Makefile, but for want of a better solution, this is good. |
36eb666
to
ff6a70d
Compare
@fabriziopandini squashed |
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 the udpates @sbueringer
/lgtm
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.
/approve
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CecileRobertMichon, fabriziopandini 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 |
What this PR does / why we need it:
This PR adds documentation on how to run e2e tests locally. This will help new contributors to avoid the somewhat cumbersome feedback loop via Prow when debugging broken e2e tests or developing new e2e tests.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #4555