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

Cleanup go unit tests #1342

Closed
1 of 2 tasks
mengqiy opened this issue Jan 31, 2020 · 7 comments
Closed
1 of 2 tasks

Cleanup go unit tests #1342

mengqiy opened this issue Jan 31, 2020 · 7 comments
Labels
lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.

Comments

@mengqiy
Copy link
Member

mengqiy commented Jan 31, 2020

There are some go unit tests in kubebuilder to test the generated files match expected.
Given that we have the golden files test, we can cleanup our go unit tests.
We can probably only keep the tests for testing the scaffolding machinery, but not for each generated file.
Maintaining go unit tests requires more efforts than the golden test, since whenever a new file is added or a file is deleted, we need to change the go test to reflect it.

2 actionable items:

  • remove go unit tests for scaffolding files
  • ensure enough coverage for the scaffolding machinery
@camilamacedo86
Copy link
Member

camilamacedo86 commented Feb 2, 2020

HI @mengqiy,

The golden tests are e2e tests and have not the same purpose as the unit tests. See that the golden will not cover all scenarios cases as the unit does. IHMO they have different purposes and both should be kept. Also, unit tests are faster and allow us to check, debug and ensure specific points locally which is very helpful for the development purpose. And then, in POV we should add more unit tests instead of just keep the golden one.

Wdyt? Is it make sense?

@Adirio
Copy link
Contributor

Adirio commented Feb 2, 2020

I think the name of unit tests brings a bit of confussion because they were really like e2e tests disguised as unit tests. They were basically creating a file in memory and checking it against the testdata folder. They were probably left-over code from before golden test was being used for this purpose. The discussion me and @mengqiy were having that brought up his issue was that these disguised tests were overlapping with the golden test and were way harder to maintain as new files required to write additional tests, where the golden test will already understand new files without requiring changes to the tests.

@camilamacedo86
Copy link
Member

camilamacedo86 commented Feb 2, 2020

Hi @Adirio,

Thank you for the clarification. I checked the tests removed at #1343 and they are like the same e2e done in the golden tests as you said. So, for one side make since removing the duplication in order to ensure the changes performed.

However, IMHO would be great we cover the project with unit tests at least since they will allow covering more scenarios cases. Also, tests implementing using go are faster and allow us to check, debug and ensure specific points locally which is very helpful for the development purpose which indeed I would advocate in prol of the usage of go test framework instead of third-party APIs as well.

May we can work on this after. Really thank you for your fantastic contributions 🥇

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 2, 2020
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jun 1, 2020
@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

@k8s-ci-robot
Copy link
Contributor

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.
Projects
None yet
Development

No branches or pull requests

5 participants