-
Notifications
You must be signed in to change notification settings - Fork 46
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
E2E test framework #443
E2E test framework #443
Conversation
deploy/yaml/kmesh.yaml
Outdated
@@ -54,7 +54,7 @@ spec: | |||
path: istio-token | |||
containers: | |||
- name: kmesh | |||
image: ghcr.io/kmesh-net/kmesh:latest |
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's better to open a new folder for e2e's yaml
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.
Good catch, I should find a way to configure HUB of the image
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.
How do you install, if using helm we can customize the image
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.
Now I apply yamls directly for convenience and I will use helm later.
# Exit immediately for non zero status | ||
set -e | ||
|
||
DEFAULT_KIND_IMAGE="kindest/node:v1.30.0@sha256:047357ac0cfea04663786a612ba1eaba9702bef25227a794b52890dd8bcd692e" |
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 you limit the kind/node image version, you will also have a limit on the kind version.
I have another option:Check for kind, then create cluster without specifying image.
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.
Istio uses this to test on compatability with different k8s version.
What we need maybe add a mechanism to test comaptability with differen istio versions
test/e2e/run_test.sh
Outdated
build_and_push_images | ||
fi | ||
|
||
go test -v -tags=integ ./test/e2e/... |
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.
go test -v -tags=integ ./test/e2e/... | |
go test -v -tags=integ $ROOT_DIR/test/e2e/... |
ROOT_DIR=$(git rev-parse --show-toplevel)
Preventing test failures by not running in the kmesh root directory.
1438e87
to
52777b8
Compare
66ff584
to
59a61a6
Compare
1fb6e0a
to
8a7ffc4
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.
Looked just a small part
docs/proposal/e2e.md
Outdated
|
||
### Motivation | ||
|
||
E2E testing is a software approach that tests an application's workflow from start to finish, simulating read-user scenarios. The main purpose of E2E testing is to validate the system as a whole,ensuring that all the individual components and integrations work together seamlessly. It helps to identify any issues or defects that may arise from the interation between different components of the appliction, ensuing the application works as expected under normal operating conditions. |
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.
s/defects/detects
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.
simulating read-user scenarios.
what does this mean?
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.
s/defects/detects
It is defetcs
actually :)
simulating read-user scenarios.
what does this mean?
updated, it is real user scenarios
.
docs/proposal/e2e.md
Outdated
|
||
1. Use [namespace](https://github.com/istio/istio/blob/master/pkg/test/framework/components/namespace/namespace.go) package to create namespace for deploying test applications. | ||
|
||
2. Use [deployment](https://github.com/istio/istio/blob/master/pkg/test/framework/components/echo/deployment/builder.go) package to build test applications. `WithClusters()` can be used to specify the clusters where the test applications should be deployed. Each call to `WithConfig()` will generate a test application with corresponding configuration. Actually we create all test applications at the beginning. In each test case, we select some suitable applications for testing. And an application can be used as both a client and a server. |
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 document if these apps do not satisfy new tests, how can developer deploy new apps
@@ -5,6 +5,8 @@ go 1.22.0 | |||
// Client-go does not handle different versions of mergo due to some breaking changes - use the matching version | |||
replace github.com/imdario/mergo => github.com/imdario/mergo v0.3.5 | |||
|
|||
replace istio.io/api => istio.io/api v1.22.0-alpha.1.0.20240620154034-5b788fec62d2 |
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.
why replace instead of bumping directly
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 we bump directly, it will use version istio.io/api v1.22.1
, there will be some interface incompatible
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 donot understand what do you mean
@@ -0,0 +1,155 @@ | |||
//go:build integ |
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.
do we really need this building tag
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 will be added automaticlly in my vscode
# Exit immediately for non zero status | ||
set -e | ||
|
||
DEFAULT_KIND_IMAGE="kindest/node:v1.30.0@sha256:047357ac0cfea04663786a612ba1eaba9702bef25227a794b52890dd8bcd692e" |
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.
Istio uses this to test on compatability with different k8s version.
What we need maybe add a mechanism to test comaptability with differen istio versions
fi | ||
|
||
# Create KinD cluster. | ||
cat <<EOF | kind create cluster --name="${NAME}" -v4 --retain --image "${IMAGE}" --config=- |
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.
can we add a multi nodes cluster, since we have daemonset
test/e2e/main_test.go
Outdated
|
||
builder := deployment.New(t). | ||
WithClusters(t.Clusters()...). | ||
/* |
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
test/e2e/main_test.go
Outdated
// AllWaypoint is a waypoint for all types | ||
AllWaypoint echo.Instances | ||
// WorkloadAddressedWaypoint is a workload only waypoint | ||
WorkloadAddressedWaypoint echo.Instances | ||
// ServiceAddressedWaypoint is a service only waypoint | ||
ServiceAddressedWaypoint echo.Instances |
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 am not sure this is suitable for kmesh, at least the names and comments are confusing
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.
removed, we don't need these fields now.
test/e2e/main_test.go
Outdated
Captured = "captured" | ||
Uncaptured = "uncaptured" | ||
WaypointImageAnnotation = "sidecar.istio.io/proxyImage" | ||
KmeshCustomWaypointImage = "ghcr.io/kmesh-net/waypoint-x86:v0.3.0" |
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 support customize it
} | ||
if src.Config().IsUncaptured() { | ||
// TODO: fix this and remove this skip | ||
t.Skip("https://github.com/istio/istio/issues/43238") |
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.
The test case is direclty copied from ambient integration test (https://github.com/istio/istio/blob/master/tests/integration/ambient/baseline_test.go#L628`).
Actually I think most the test cases there should be copied and used direcly. Because we need to fully match ambient's capabilities and be transparent to isito and users.
if opt.Scheme != scheme.HTTP { | ||
return | ||
} | ||
if !dst.Config().HasServiceAddressedWaypointProxy() { |
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.
Can you remove this istio special concept
Signed-off-by: YaoZengzeng <[email protected]>
Signed-off-by: YaoZengzeng <[email protected]>
Signed-off-by: YaoZengzeng <[email protected]>
Signed-off-by: YaoZengzeng <[email protected]>
Signed-off-by: YaoZengzeng <[email protected]>
Signed-off-by: YaoZengzeng <[email protected]>
Signed-off-by: YaoZengzeng <[email protected]>
Signed-off-by: YaoZengzeng <[email protected]>
Signed-off-by: YaoZengzeng <[email protected]>
Signed-off-by: YaoZengzeng <[email protected]>
Signed-off-by: YaoZengzeng <[email protected]>
Signed-off-by: YaoZengzeng <[email protected]>
Signed-off-by: YaoZengzeng <[email protected]>
Signed-off-by: YaoZengzeng <[email protected]>
Signed-off-by: YaoZengzeng <[email protected]>
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 super good, i will try with it. And understand it deeper
fi | ||
|
||
if [[ -z "${SKIP_SETUP:-}" ]]; then | ||
setup_kind_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.
If i have already installed kind cluster and istio, how can i reuse them
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.
You can use flags to skip some steps, the specific usage method has been added to the proposal.
Signed-off-by: YaoZengzeng <[email protected]>
Signed-off-by: YaoZengzeng <[email protected]>
@hzxuzhonghu PTAL |
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hzxuzhonghu 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 type of PR is this?
/kind feature
What this PR does / why we need it:
E2E test framework to make the project more robust
Which issue(s) this PR fixes:
Fixes most part of #137
Special notes for your reviewer:
Does this PR introduce a user-facing change?: