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

🌱 Tests cleanup #1933

Merged
merged 1 commit into from
Feb 14, 2021
Merged

Conversation

Adirio
Copy link
Contributor

@Adirio Adirio commented Jan 8, 2021

Description

The following list defines all the checks and tests that we currently have, with some consideration on prerequisites and under which conditions are being executed (go version, devide OS/arch, and kubernetes version).

Lint

  • Description: verifies the code style.
  • Prerequisites: none.
  • Make: make lint to report the errors or make lint-fix to solve them.
  • CI: yes (reporting, GH Actions).
  • Parameters: single execution.

Unit tests

  • Description: unit tests defined in *_test.go files run by go test ... that test the different parts of the library isolated.
  • Prerequisites: lint. *1
  • Make: make test-unit.
  • CI: yes (GH Actions & Prow). *2
  • Parameters: multiple executions based on device OS/arch as right now. *3

Coverage *4

  • Description: obtains coverage info of unit tests and upload it to coveralls.
  • Prerequisites: unit tests.
  • Make: make test-coverage obtains the coverage info.
  • CI: yes (GH Actions).
  • Parameters: single execution.

Integration tests *5

  • Description: runs some basic kubebuilder commands to test the CLI.
  • Prerequisites: unit tests.
  • File: test/integration.sh.
  • Make: test-integration.
  • CI: yes (Prow).
  • Parameters: single execution.

Testdata check

  • Description: verifies that the code under the testdata directory is up to date.
  • Prerequisites: lint. *6
  • File: test/testdata/check.sh.
  • Make: make check-testdata.
  • CI: yes (GH Actions).
  • Parameters: single execution. *7

Testdata tests

  • Description: executes the unit tests of the scaffolded projects in the testdata directory.
  • Prerequisites: integration tests. *8
  • File: test/testdata/test.sh.
  • Make: make test-testdata.
  • CI: yes (Prow).
  • Parameters: single execution. *7

E2E tests

  • Description: end-to-end tests.
  • Prerequisites: none. *9
  • File: test/e2e/local.sh or test/e2e/ci.sh.
  • Make: make test-e2e-local or make test-e2e-ci.
  • CI: yes (Prow).
  • Parameters: multiple executions for several kubernetes versions.

Notes:

  1. Linting is not really a prerequisite for unit tests in the most strict sense, but as it is pretty fast, I don't think that it matters that much.
  2. We should remove the overlap between GH Actions and Prow.
  3. May be interesting to also test for multiple go versions if we do support more than one major go version.
  4. We may be able to obtain the coverage info directly from one of the unit tests instead of reruning the tests again for coverage but this may be a bit tricky as we only need to upload one coverage report but unit tests are being executed for different OS/arch.
  5. Integration tests are pretty old and have very few cases tested. Testdata tests below are also integrations tests that have way more cases tested and the generated code is actually checked. We should probably remove the old integration tests in favor of the testdata tests.
  6. In order to check the testdata folder we need to regenerate it, which requires a running kubebuilder CLI, so testdata check should have as prerequisite the unit tests, not the lint.
  7. It may be interesting to run testdata check and tests for several go versions and device OS/arch.
  8. We should set check testdata as the prerequisite for test testdata, which would require to move this test to GH Actions.
  9. I think that we should set as prerequisites all the previous tests (including unit tests, and testdata tests) which would require to move this tests to GH Actions.

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 8, 2021
@Adirio Adirio closed this Jan 9, 2021
@Adirio Adirio reopened this Jan 9, 2021
@Adirio Adirio force-pushed the cleanup-tests branch 9 times, most recently from f500ade to 4807df2 Compare January 9, 2021 19:58
@Adirio Adirio closed this Jan 9, 2021
@Adirio Adirio reopened this Jan 9, 2021
@Adirio Adirio force-pushed the cleanup-tests branch 5 times, most recently from e284d27 to 794c33c Compare January 10, 2021 00:08
@Adirio Adirio marked this pull request as ready for review January 10, 2021 00:09
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 10, 2021
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 22, 2021
@Adirio
Copy link
Contributor Author

Adirio commented Jan 22, 2021

Yep, but if we add config/v3 we may need to modify the generate testdata file so I will rebase after that.

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 26, 2021
@Adirio Adirio force-pushed the cleanup-tests branch 2 times, most recently from 0d33217 to 36b53f5 Compare January 26, 2021 12:09
@Adirio
Copy link
Contributor Author

Adirio commented Jan 26, 2021

Rebased. There is a single really small conflict with config/v3, so the order in which they are merged doesn't really matter.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 30, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 31, 2021
@Adirio Adirio force-pushed the cleanup-tests branch 2 times, most recently from acc53ad to 7d55bb3 Compare January 31, 2021 14:42
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 4, 2021
@Adirio Adirio force-pushed the cleanup-tests branch 2 times, most recently from 73b9968 to 3516ec2 Compare February 4, 2021 23:18
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 5, 2021
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 11, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 12, 2021
Signed-off-by: Adrian Orive <[email protected]>
Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

It shows fine for me

/lgtm

Great job. 🥇

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 14, 2021
@k8s-ci-robot k8s-ci-robot merged commit 6224a06 into kubernetes-sigs:master Feb 14, 2021
@Adirio Adirio deleted the cleanup-tests branch February 14, 2021 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants