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

📖 Improve testing guidelines #6112

Merged

Conversation

fabriziopandini
Copy link
Member

What this PR does / why we need it:
Improving the current testing guidelines to better reflect how we act + keeping track of some considerations around testing that popped up during v1.1 work

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 11, 2022
Copy link
Contributor

@mdbooth mdbooth left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 11, 2022
Copy link
Contributor

@killianmuldoon killianmuldoon left a comment

Choose a reason for hiding this comment

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

Looks good - this is definitely more in line with our actual practices around testing.

docs/book/src/developer/testing.md Outdated Show resolved Hide resolved
docs/book/src/developer/testing.md Outdated Show resolved Hide resolved

Instead, mocking is much more relevant for infrastructure providers; in order to address the issue
some providers can use simulators reproducing the behaviour of a real infrastructure providers (e.g CAPV);
if this is not possible, a viable solution is to use mocks (e.g CAPA).
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we post a link to an example of where mocks are used?

Copy link
Member Author

Choose a reason for hiding this comment

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

The goal of this paragraph is to create awareness on the topic and give a pointer to where this is implemented outside of the CAPI repo, but I'm personally -1 to add a link to unit test code in another repository given that this will be brittle.

Comment on lines 29 to 31
While testing core Cluster API contributors should ensure that the code works with any providers, and thus it is required
to not use any specific provider implementation. Instead, the so-called generic providers should be used because they
implement the plain Cluster API contract, thus preventing developers to make assumptions we cannot rely on.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
While testing core Cluster API contributors should ensure that the code works with any providers, and thus it is required
to not use any specific provider implementation. Instead, the so-called generic providers should be used because they
implement the plain Cluster API contract, thus preventing developers to make assumptions we cannot rely on.
When writing tests core Cluster API contributors should ensure that the code works with any providers, and thus it is required
to not use any specific provider implementation. Instead, the so-called generic providers e.g. "GenericInfrastructureCluster" should be used because they
implement the plain Cluster API contract. This prevents tests from relying on assumptions that may not hold true in all cases.

Possibly add a link to the generic providers in the code? Or at least the name of the type so they can be searched.

Copy link
Member Author

@fabriziopandini fabriziopandini Feb 11, 2022

Choose a reason for hiding this comment

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

if you have in mind an example that can be used as a reference happy to add it; personally I'm a little bit concerned about adding a link because what we have is really fragmented. Even if not ideal, I prefer contributors to ask for guidance on this point so we can weigh the pros and cons case by case; also, using the v1.1 timeframe as a benchmark, changes of this type are not really frequent.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is fine, given that a link might be more confusing. Adding the name of the example type, i.e. GenericInfrastructureCluster, lets people search for it themselves in the codebase to see the implementation

docs/book/src/developer/testing.md Outdated Show resolved Hide resolved
docs/book/src/developer/testing.md Outdated Show resolved Hide resolved
docs/book/src/developer/testing.md Outdated Show resolved Hide resolved
docs/book/src/developer/testing.md Outdated Show resolved Hide resolved
docs/book/src/developer/testing.md Outdated Show resolved Hide resolved
docs/book/src/developer/testing.md Outdated Show resolved Hide resolved
@fabriziopandini fabriziopandini changed the title 📖 iImprove testing guidelines 📖 Improve testing guidelines Feb 11, 2022
@pydctw
Copy link

pydctw commented Feb 11, 2022

Thanks for updating the testing guidelines and including discussion about mocking and testing in layers.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 11, 2022
Copy link
Contributor

@killianmuldoon killianmuldoon left a comment

Choose a reason for hiding this comment

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

/lgtm

@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, 2022
Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 14, 2022
@k8s-ci-robot k8s-ci-robot merged commit 4ecff53 into kubernetes-sigs:main Feb 14, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.2 milestone Feb 14, 2022
@sbueringer
Copy link
Member

Thx!
/lgtm
from my side too

@fabriziopandini fabriziopandini deleted the improving-testing-guidelines branch March 1, 2022 09: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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants