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 unit test for vineyardctl #1396

Closed
wants to merge 0 commits into from
Closed

add unit test for vineyardctl #1396

wants to merge 0 commits into from

Conversation

zhuyi1159
Copy link
Contributor

What do these changes do?

This is my first attempt to submit a PR, and I have added a main_test.go test file for vineyardctl , but there are multiple folders such as 'create' and 'delete' in the cmd directory, and each folder has multiple files. I don't know if I should write a test file for each of these files. I would be very grateful if my doubts can be answered.

Related issue number

Fixes #1301

@stoat-app
Copy link

stoat-app bot commented May 25, 2023

Easy and customizable dashboards for your build system. Learn more about Stoat ↗︎

Static Hosting

Name Link Commit Status
spill-logs (job: e2e-tests-failover) Visit e297c80
sidecar-logs (job: e2e-tests-failover) Visit e297c80
failover-logs (job: e2e-tests-failover) Visit e297c80
workflow-logs (job: e2e-tests-failover) Visit e297c80
serialize-logs (job: e2e-tests-failover) Visit e297c80
vineyardctl-logs (job: e2e-tests-failover) Visit e297c80
assembly-local-logs (job: e2e-tests-failover) Visit e297c80
repartition-dask-logs (job: e2e-tests-failover) Visit e297c80
assembly-distributed-logs (job: e2e-tests-failover) Visit e297c80
autogenerated-helm-chart-logs (job: e2e-tests-failover) Visit e297c80
scheduler-outside-cluster-logs (job: e2e-tests-failover) Visit e297c80
deploy-raw-backup-and-recover-logs (job: e2e-tests-failover) Visit e297c80

debug

@github-actions
Copy link
Contributor

github-actions bot commented May 25, 2023

🎊 PR Preview 8a14dbc4324ea61c995467d2707a960cd2fda80c has been successfully built and deployed to https://deploy-preview-pr-1396--v6d.netlify.app
🤖 By netlify

@dashanji
Copy link
Member

Hi, @zhuyi1159, thanks for the contribution and It's very nice that you can attempt to propose a PR. However, the current pr is at a very early stage, and I suggest moving it to draft and reopening it when it's ready for review.

As for your question, we highly recommend writing the related unit test for each file to improve the test coverage, and the unit test coverage should be more than 80%.

Some tips for you:
There are many golang unit test frameworks, such as gotests, ginkgo and etc. You can choose one you like to go on.

@zhuyi1159
Copy link
Contributor Author

Thank you for your guidance. Now I have generated corresponding test files (except for the utils subfolder) for almost every go file in the cmd directory by using gotests and resubmitted a commit. Does this comply with regulations? If there are any non-compliance areas, please let me know and I will be very happy to make corrections

@dashanji
Copy link
Member

The framework looks good to me, but you need to fulfill the test cases in the blank named // TODO: Add test cases..

name string
want *cobra.Command
}{
// TODO: Add test cases.
Copy link
Member

@dashanji dashanji May 26, 2023

Choose a reason for hiding this comment

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

Insert your test cases in all test files.

@zhuyi1159
Copy link
Contributor Author

zhuyi1159 commented May 29, 2023

I have completed the unit tests for 'create' and 'delete' two folders, and running the 'go test' command shows that they pass. However, because I don't have much knowledge about Go language and Kubernetes, it took me a long time to write the test cases, and I'm not sure if they are compliant even though the 'go test' command passed. Is there any technique for writing test cases? Also, the deadline for submitting the application is approaching, can I submit my OSPP application first?

@dashanji
Copy link
Member

You'd better to understand what the command will do and then write the unit tests. No problem, just go ahead for OSPP.

@netlify
Copy link

netlify bot commented Jul 16, 2023

Deploy Preview for v6d ready!

Name Link
🔨 Latest commit b438f0e
🔍 Latest deploy log https://app.netlify.com/sites/v6d/deploys/64c325e2091b0e00088b3883
😎 Deploy Preview https://deploy-preview-1396--v6d.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@sighingnow sighingnow left a comment

Choose a reason for hiding this comment

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

Use English for comments.

@dashanji
Copy link
Member

dashanji commented Jul 18, 2023

@zhuyi1159 The update looks better than the original one, but the current design only the default configurations of flags. It's better to add some test cases for the command itself. There is an example

https://github.com/dashanji/v6d/blob/f876b6f3d63693186885160516b4c63d62cb614a/k8s/cmd/commands/deploy/deploy_vineyard_deployment_test.go#L35-L86

@zhuyi1159 zhuyi1159 closed this Jul 28, 2023
@zhuyi1159 zhuyi1159 force-pushed the main branch 2 times, most recently from 8bda147 to b438f0e Compare July 28, 2023 02:20
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.

Add unit test for vineyardctl and vineyard operator
3 participants