-
Notifications
You must be signed in to change notification settings - Fork 165
e2e upgrade #2938
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
base: main
Are you sure you want to change the base?
e2e upgrade #2938
Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lkingland 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 |
94c8089 to
7512faf
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2938 +/- ##
==========================================
- Coverage 59.37% 54.75% -4.63%
==========================================
Files 134 134
Lines 13500 13494 -6
==========================================
- Hits 8016 7389 -627
- Misses 4540 5198 +658
+ Partials 944 907 -37
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
b68e41f to
e398349
Compare
5600e62 to
e2184c3
Compare
9ac60f0 to
b48eef4
Compare
|
It ain't perfect, but I would prefer we merge this sooner rather than later as folks need this, and we can open new PRs for any niggling issues. |
|
|
||
| func (i info) XML(w io.Writer) error { | ||
| return xml.NewEncoder(w).Encode(i) | ||
| return errors.New("XML export not currently supported") |
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.
XML breakes trying to export anonymous structs. And XML is evil.
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.
Maybe it makes sense to remove it here and in format.go?
| * `FUNC_REPO_REF` affects which github repo will be used to fetch tekton tasks for on cluster build. Default: `knative/func`. | ||
| * `FUNC_REPO_BRANCH_REF` affects which github branch will be used to fetch tekton tasks for on cluster build. Default: `main`. |
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.
Apparently these are completely unused.
|
|
||
| .PHONY: update-runtime-python | ||
| update-runtime-python: | ||
| # Nothing to update for Python |
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.
Explicitly stating why it's not here.
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.
Is this desired? Why not remove it?
|
|
||
| .PHONY: check | ||
| check: $(BIN_GOLANGCI_LINT) ## Check code quality (lint) | ||
| check: check-lint check-goimports check-misspell check-whitespace check-eof ## Check code quality (comprehensive) |
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.
Adds checkers to be as close to those from knative automations such that running make check locally is a reasonable approximation.
| STERN=$(find_executable "stern") | ||
| KN=$(find_executable "kn") | ||
| JQ=$(find_executable "jq") | ||
| KUBECTL=$(find_executable "kubectl" || true) |
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.
not all scripts require all binaries, so this is opportunistic; allowing failure to happen at time of use if not found.
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.
A short comment here telling exactly that would be great!
05ad84d to
4cc4d72
Compare
4052fcd to
79ef180
Compare
install git server in workflow disable dapr service-to-service test only run private repo test on Linux test full script etc test failure fixes remote s2i builder test full stuffs python functions fixes update func-python e2e wait by duration wait options with extended timeouts for java runtimes import common in git-server.sh disable make buffering import common to dump-logs.sh update dump-logs.sh to handle missing binaries update common.sh to be resilient to missing binaries fix: gitlab tests (retries and skip in CI) full test target - script reenable http default matrix template support user vs system config in registry setup script e2e_test.go increase default timeout full test target add CleanFS option unified github workflow renamed env var standardized envs for int tests dedicated Podman scripts integration test name prefix registry.sh skip docker restart add missing io import to gitlab_int_test.go improve prechecks
79ef180 to
c069c6f
Compare
|
PR needs rebase. 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-sigs/prow repository. |
twoGiants
left a comment
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.
Looks pretty awesome! What a great refactoring! 🥇 😸
Learned a lot about the project reviewing it. And I always like it in PRs when many files are removed and only a few added. It feels more cleaned up then. 🧹 😆 🧹
I have 3 files to review left. Will continue on Monday.
Most, if not all comments are non-blocking.
| - uses: actions/checkout@v4 | ||
| - uses: knative/actions/setup-go@main | ||
|
|
||
| - name: Free up disk space |
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.
Nice one! 🥇
This should be a well maintained Github Action...just searched a bit and there are a few, but not all maintained. This one looks maintained.
| # Operating system temporary files | ||
| .DS_Store | ||
|
|
||
| # TODO: Update this test to be from a temp directory with a hard-coded impl: |
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 would not commit # TODO: .... Better to create an issue and reference it here, so it can be picked up by someone.
# Issue #1234: ...Same for the TODO below.
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.
Ideally, yes, but there's an extenuating circumstance: 1) the Knative bot auto-deletes issues after a certain time, and 2) that creates an implicit dependency between our code and the GitHub issues database.
I'm not sure if this is standard practice, but I tend to say:
# TODO: Nonfatal, to be done in the future, possibly (a known, acceptable capitulation)
# FIXME: Fatal, should not be committed until handled.
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.
Not a fan tbh. It "never" gets picked up and it lacks clarity. Is someone working on it? Should I resolve it? Do I leave it? Whats the priority?
- Can the Knative bot be configured not to do that if the reference in the code is there?
- What issue do you see with having this dependency in the code to the Github issues (a tool)?
- We could also remove the
TODOand have an issue in our internal JIRA. Then it won't be deleted and triaged at some point.
|
|
||
| .PHONY: update-runtime-python | ||
| update-runtime-python: | ||
| # Nothing to update for Python |
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.
Is this desired? Why not remove it?
| Please note that the version of `yq` required is installed via `pip3 install yq` or `brew install python-yq` | ||
|
|
||
|
|
||
| ### Allocate |
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.
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.
Fortunately that's no longer a manual requirement because the ./hack/binaries.sh script now installs all necessary binaries in an isolated environment. That document needs to be (will be) completely rewritten.
|
|
||
| func (i info) XML(w io.Writer) error { | ||
| return xml.NewEncoder(w).Encode(i) | ||
| return errors.New("XML export not currently supported") |
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.
Maybe it makes sense to remove it here and in format.go?
|
|
||
| # Install bash 4+ (macOS ships with bash 3.x) | ||
| echo "${blue}Installing bash...${reset}" | ||
| brew install bash |
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.
Is this really needed on macOS? It ships with zsh as a default since 2019.
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 boils down to the hashbang: If we specify zsh, that rules out many linux development machines without zsh (our primary development platform). If we use bash (the universal default), then we need to ensure it's the updated GNU core utils version or our scripts break.
| return name | ||
| } | ||
|
|
||
| func checkTestEnabled(t *testing.T) { |
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 function is unused. Also setupNS, createSimpleGoProject. Comment in line 47 can also be removed.
| t.Log("created svc:", svc.Name) | ||
|
|
||
| // wait for service to start | ||
| // TODO: Replace with proper readiness check. This sleep gives the deployment |
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 would replace the TODO with a Github issue and reference it here.
| // TestInt_Invoke_ServiceToService ensures that a Function can invoke another | ||
| // service via localhost service discovery api provided by the Dapr sidecar. | ||
| func TestInt_Invoke_ServiceToService(t *testing.T) { | ||
| t.Skip("TODO: dapr appears to be borked") |
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 would add a Github issue and reference it here and replace the TODO with it.
|
|
||
| // Skip in CI due to persistent timeout issues regardless of allocated time | ||
| // Note it does indeed run locally. | ||
| // TODO: Investigate why GitLab webhook builds are not completing in CI |
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 would create a Github issue and reference it here and replace the TODO with it. Then it can be picked up by someone 😸 .
| - `./hack/binaries.sh` Fetch binaries into `./hack/bin` | ||
| - `./hack/registry.sh` (once) Configure insecure local registriry | ||
| - `./hack/cluster.sh` Create a cluster and kube config in `./hack/bin` | ||
| - `make test-all` Run all tests using these binaries and cluster |
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.
| - `make test-all` Run all tests using these binaries and cluster | |
| - `make test-full` Run all tests using these binaries and cluster |
|
|
||
| // DefaultNamespace for E2E tests is that used by default in the | ||
| // CLI being tested. | ||
| DefaultNamespace = cmd.DefaultNamespace |
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 resolves to "default" aways. It would be nice if this could be configurable (i.e. FUNC_E2E_NAMESPACE) or resolve to current namespace on cluster
|
|
||
| // Kubeconfig - the kubeconfig to pass ass KUBECONFIG env to test | ||
| // environments. | ||
| Kubeconfig = getEnvPath("FUNC_E2E_KUBECONFIG", "", DefaultKubeconfig) |
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.
| Kubeconfig = getEnvPath("FUNC_E2E_KUBECONFIG", "", DefaultKubeconfig) | |
| Kubeconfig = getEnvPath("FUNC_E2E_KUBECONFIG", "KUBECONFIG", DefaultKubeconfig) |
it could also rely on KUBECONFIG env var (just suggestion)
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 thought that for that case the FUNC_E2E_KUBECONFIG should be used.
| } | ||
| defer l.Close() | ||
| return l.Addr().String(), nil | ||
| } |
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.
Suggestion: what about split this file per test category.
- e2e_test.go -> consts, init, utils, waitFor funcs,...
- e2e_core_test.go -> TestCore_*
- e2e_metadata_test.go -> TestMetadata_*
- e2e_[remote|podman|matrix]_test.go -> and so on...
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.
Agree, this would be great!
| clean(t, name, DefaultNamespace) | ||
| }() | ||
|
|
||
| if !waitForEcho(t, fmt.Sprintf("http://%v.default.localtest.me", 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.
Maybe it could have cluster domain as environment variable either? it would make test more cluster agnostic.
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.
But the cluster created for this e2e tests is using this cluster domain. Not sure if someone would create/use their own custom setup with a different DNS config just for the tests.
Wdyt?
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.
Yes these tests are strict to the Kind cluster created for CI, but I would (I probably will) run these test set with OCP.
| if err := newCmd(t, "init", "-l=go").Run(); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if err := newCmd(t, "deploy", "--remote", "--builder=pack", "--registry=registry.default.svc.cluster.local:5000/func").Run(); err != nil { |
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.
Registry is hardcoded here (there are other occurencies). Maybe this could be a variable (i.e ClusterInternalRegistry) also configurable by an env var?
twoGiants
left a comment
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.
Great job 😸 🚀 🎸 🥇!
This is a big improvement to what is there and the structure is nice, clean and clear.
I haven't found anything critical. Just typos, TODOs which can be extracted into issues and a few minor nice-to-haves.
I am mostly done. The last file I am still reviewing are the e2e-test.go which is the biggest one. I will submit what I have so far for it and the rest today/tomorrow.
| debuggable) locally by a developer, in addition to remotely in CI as | ||
| acceptance criteria for pull requests. | ||
|
|
||
| ## Runnning E2Es locally: a Quick-start |
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.
| ## Runnning E2Es locally: a Quick-start | |
| ## Running E2Es locally: a Quick-start |
| ## Runnning E2Es locally: a Quick-start | ||
|
|
||
| - `./hack/binaries.sh` Fetch binaries into `./hack/bin` | ||
| - `./hack/registry.sh` (once) Configure insecure local registriry |
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.
| - `./hack/registry.sh` (once) Configure insecure local registriry | |
| - `./hack/registry.sh` (once) Configure insecure local registry |
| - `binaries.sh`: Installs executables needed for cluster setup and | ||
| configuration into hack/bin. | ||
|
|
||
| - `registry.sh`: Configures the local Podman or Docker to allow unencrypted |
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 there be also a --revert or --undo option for this script?
| package, not the directory from which `go test` was run. | ||
|
|
||
| `FUNC_E2E_PLUGIN`: if set, the command run by the tests will be | ||
| `${FUNC_E2E_BIN} func`, allowing for running all tests when func is installed |
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.
| `${FUNC_E2E_BIN} func`, allowing for running all tests when func is installed | |
| `${FUNC_E2E_BIN} ${FUNC_E2E_PLUGIN}`, allowing for running all tests when func is installed |
|
|
||
| ## Running | ||
|
|
||
| From the root of the repository, run `make test-all`. This will compile |
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.
From the root of the repository, run make test-full. This will compile
| } | ||
| } | ||
|
|
||
| // waitForEcho returns true if there is service at the given addresss which |
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.
| // waitForEcho returns true if there is service at the given addresss which | |
| // waitForEcho returns true if there is service at the given address which |
| // If the Function returns a 500, it is considered a positive test failure | ||
| // by the implementation and retries are discontinued. | ||
| // | ||
| // TODO: Implement a --output=json flag on `func run` and update all |
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 would create a Github issue for this TODO and reference it here.
| for time.Since(startTime) < cfg.timeout { | ||
| attemptCount++ | ||
| time.Sleep(cfg.interval) | ||
| // t.Logf("GET %v\n", address) |
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.
Remove or uncomment.
| // t.Logf("GET %v\n", address) | |
| t.Logf("GET %v\n", address) |
| } | ||
|
|
||
| // Wait for echo | ||
| if !waitForEcho(t, "http://"+address) { |
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 this run into flakes a lot? If not then all good!
If yes, then a more specific implementation using kubectl which waits for the pod and service availability and then sends an echo would be an improvement.
But not here in this PR of course. It can be a help-wanted issue.
| clean(t, name, DefaultNamespace) | ||
| }() | ||
|
|
||
| if !waitForEcho(t, fmt.Sprintf("http://%v.default.localtest.me", 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.
But the cluster created for this e2e tests is using this cluster domain. Not sure if someone would create/use their own custom setup with a different DNS config just for the tests.
Wdyt?
twoGiants
left a comment
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.
Hi @lkingland! Finished the e2e_test.go. That was the biggest one! 😅
Looks good to me!
I have only found some typos and propose to cleanup a bit here and there, extract issues from TODOs and reduce duplication. Can all be done in a follow up, except the TODOs, those I would remove now and create issues. But you decide, its all good 😸
| } | ||
|
|
||
| // Assert | ||
| f, err := fn.NewFunction(root) |
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 would probably assert a bit more here. Because it's an e2e test I would check that what I expect was actually generated and has what I expect in it.
| } | ||
|
|
||
| // TestCore_Deploy_Template ensures that the system supports creating | ||
| // functions based off templates in a remote repository. |
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.
| // functions based off templates in a remote repository. | |
| // functions based of templates in a remote repository. |
| // repo: github.com/functions-dev | ||
| // runtime: go | ||
| // template: http (the default. can be changed with --template) | ||
| if err := newCmd(t, "init", "-l=go", "--repository=https://github.com/functions-dev/func-e2e-tests").Run(); err != nil { |
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.
https://github.com/functions-dev/func-e2e-tests => hosted on the gitlab instance in the bootstrapped cluster would be nice and more independent 😸 . Maybe also a help-wanted issue for later.
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.
It could be even be hosted on the git-server available for remote tests.
| } | ||
|
|
||
| // Validate that the name matches what we expect | ||
| if instance.Name != 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.
Describe shows more than just the name. Maybe we could assert for the entire content?
| } | ||
| }() | ||
|
|
||
| // TODO: complete implementation of `func run --json` structured output |
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 would put the TODO in a Github issue and reference it here.
| if err == nil { | ||
| return false // no error is not an abnormal error. | ||
| } | ||
| t.Helper() |
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.
| if err == nil { | |
| return false // no error is not an abnormal error. | |
| } | |
| t.Helper() | |
| t.Helper() | |
| if err == nil { | |
| return false // no error is not an abnormal error. | |
| } |
|
|
||
| // setSecret creates or replaces a secret. | ||
| func setSecret(t *testing.T, name, ns string, data map[string][]byte) { | ||
| ctx := context.Background() |
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.
| ctx := context.Background() | |
| t.Helper() | |
| ctx := context.Background() |
|
|
||
| // setConfigMap creates or replaces a configMap | ||
| func setConfigMap(t *testing.T, name, ns string, data map[string]string) { | ||
| ctx := context.Background() |
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.
| ctx := context.Background() | |
| t.Helper() | |
| ctx := context.Background() |
| // There is currently a bug in delete which hangs for several seconds | ||
| // when deleting a Function. This adds considerably to the test suite | ||
| // execution time. Tests are written such that they are not dependent | ||
| // on a clean exit/cleanup, so this step is skipped for speed. |
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 comment is a bit misleading: ..., so this step is skipped for speed. Where is this step skipped?
| func logImageStats(t *testing.T, name string, phase string) { | ||
| t.Helper() | ||
|
|
||
| // Get image size for this specific function |
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.
| // Get image size for this specific function | |
| // Get image name for this specific function |
Unifies E2E tests and workflows
/kind cleanup
Primary Changes:
make test-fulltarget runs all tests (unit, integration, E2E).Notable Name Changes: the scripts in ./hack were renamed to be simple nouns:
hack/cluster.sh
hack/binaries.sh
hack/images.sh
hack/gitlab.sh