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.

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

add consumer test for e2e #108

Merged
merged 3 commits into from
Jun 25, 2024
Merged

Conversation

yanmxa
Copy link
Contributor

@yanmxa yanmxa commented May 31, 2024

Signed-off-by: myan [email protected]

Add the e2e test case for CURD on consumers

@yanmxa yanmxa changed the title add e2e test for api add consumer test for e2e Jun 11, 2024
@yanmxa
Copy link
Contributor Author

yanmxa commented Jun 11, 2024

/assign @clyang82 @skeeey @qiujian16 @morvencao

apiServerAddress string
kubeconfig string
consumer_name string
kubeClient *kubernetes.Clientset
apiClient *openapi.APIClient
helper *test.Helper
Copy link
Contributor

@morvencao morvencao Jun 11, 2024

Choose a reason for hiding this comment

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

@clyang82
I'm curious why we need helper in e2e testing? It's mainly used by integration test to set up maestro servers in process, but for e2e testing, the maestro server is deployed in kubernetes cluster, the only use of helper is to create openapi resource?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. only use it to create openapi resource.

Makefile Outdated Show resolved Hide resolved
apiServerAddress string
kubeconfig string
consumer_name string
kubeClient *kubernetes.Clientset
apiClient *openapi.APIClient
helper *test.Helper
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. only use it to create openapi resource.

got = true
}
}
Expect(got).To(BeTrue())
Copy link
Contributor

@clyang82 clyang82 Jun 13, 2024

Choose a reason for hiding this comment

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

how to guarantee the consumer is there? the test order is random. if run this test at the beginning, the consumer should not exist. right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since all the specs are located in an Ordered Consumer , all the specs are guaranteed to run sequentially, in the order they are written.
refer: https://onsi.github.io/ginkgo/#ordered-containers

Copy link
Contributor Author

@yanmxa yanmxa Jun 13, 2024

Choose a reason for hiding this comment

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

For this case, we already have generated a consumer before running the e2e test. So the consumer must be there~

export consumer_name=$(curl -k -X POST -H "Content-Type: application/json" https://${external_host_ip}:30080/api/maestro/v1/consumers -d '{}' | jq '.id')

Copy link
Contributor

Choose a reason for hiding this comment

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

How about the case you want to run it in an existing environment? Do you require create the consumer as a pre-requirement? maybe just adjust your order to create/get/patch

@yanmxa yanmxa requested review from clyang82 and morvencao June 13, 2024 08:14
@yanmxa yanmxa force-pushed the br_e2e_test branch 5 times, most recently from 44013a8 to db488d7 Compare June 19, 2024 09:31
Makefile Outdated Show resolved Hide resolved
got = true
}
}
Expect(got).To(BeTrue())
Copy link
Contributor

Choose a reason for hiding this comment

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

How about the case you want to run it in an existing environment? Do you require create the consumer as a pre-requirement? maybe just adjust your order to create/get/patch

@yanmxa yanmxa force-pushed the br_e2e_test branch 4 times, most recently from d2bd913 to 71b7b45 Compare June 24, 2024 09:04
Copy link
Contributor

@clyang82 clyang82 left a comment

Choose a reason for hiding this comment

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

LGTM

@clyang82
Copy link
Contributor

/ok-to-test

Signed-off-by: myan <[email protected]>

update the source client

Signed-off-by: myan <[email protected]>

rebase

Signed-off-by: myan <[email protected]>

fix

Signed-off-by: myan <[email protected]>

reply review

Signed-off-by: myan <[email protected]>
@yanmxa
Copy link
Contributor Author

yanmxa commented Jun 25, 2024

/ok-to-test

yanmxa added 2 commits June 25, 2024 17:02
@yanmxa
Copy link
Contributor Author

yanmxa commented Jun 25, 2024

/ok-to-test

@yanmxa
Copy link
Contributor Author

yanmxa commented Jun 25, 2024

/cc @clyang82

@clyang82 clyang82 merged commit b7bddd5 into openshift-online:main Jun 25, 2024
5 checks passed
@clyang82
Copy link
Contributor

Thanks @yanmxa for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants