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 the requirement that GA/stable APIs must have conformance tests #1806

Merged
merged 1 commit into from
Mar 1, 2018

Conversation

bgrant0607
Copy link
Member

Add the requirement that GA/stable APIs must have conformance tests, if/as appropriate.

The intent is to stop the bleeding on features progressing to GA without conformance tests. There are some details to work out, but I don't think they belong in this doc. I'm just trying to capture the intent here.

cc @kubernetes/sig-architecture-api-reviews @jagosan @WilliamDenniss

@k8s-ci-robot k8s-ci-robot added sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 15, 2018
@smarterclayton
Copy link
Contributor

I don’t see any compelling reason not to do this in 1.10.

Lgtm but will let others weigh in as well.

@duglin
Copy link

duglin commented Feb 16, 2018

@brahmaroutu can you add a link to the doc that describes the metadata that conformance tests are support to include? @bgrant0607 is this api_changes doc the right (and only spot) it should go?

@liggitt
Copy link
Member

liggitt commented Feb 16, 2018

This PR was cited in a recent email discussing CNCF funding of "missing" conformance tests. A coverage number of 11% was discussed, but much of the remaining coverage already exists in the unit and integration test suites. Is it a goal to move those to e2e tests (which is where conformance tests currently live)?

It would be good to clarify what coverage is expected in conformance tests:

  • every API verb on every API version on every API resource? (which is what the CNCF email implied)
  • every possible value of enumerated API fields?
  • something else?

@bgrant0607
Copy link
Member Author

@duglin: Let's create a central hub for conformance-related info. We could then link to that from here and elsewhere, as appropriate.

@liggitt: I didn't intend to get into the technical details of how to add conformance tests in this PR, much as it doesn't describe how to write other kinds of tests, either. We need other documents to provide answers to your questions.

The CNCF-funded effort isn't the only part of what we need to do to increase coverage.

The current set of conformance tests are end-to-end tests that were originally tagged with [Conformance] because they thought the tests exercised portable functionality. It started as a simple way to vet ports of kube-up.sh to new providers and environments, and new cloudprovider implementations.

Unsurprisingly, there are many coverage-related shortcomings in the current test suite:

  • A relatively small, skewed set of functionality is disproportionately covered, biased by the people who thought to add them.
  • Functionality tested by different kinds of tests, such as unit, component, or integration tests, is not covered. Our desire to move to more targeted tests is in direct tension with the need to apply conformance tests to full platforms.
  • The tests were not really designed to be used to test platform behaviors in all reasonable environments, such as outside North America or off the public Internet.
  • There are no documented rules about what should be covered and what shouldn’t be.
  • We don’t have good coverage measurement.
  • Basic functionality is mostly accidentally covered, as side effects of other tests.
  • Newer functionality is mostly not covered.

Some specific examples:

  • The newer workload APIs are not covered. Probably they, and many other APIs, have tests that could be converted to conformance tests.
  • The node conformance tests aren't included.

We will need to address such issues. For example, we're going to need a new test framework that abstracts how such tests are executed so that they can be launched on full clusters as well as more targeted, efficient, stable tests.

But I don't think solving all of those problems should block the policy. We need to start chipping away at this.

In any case, even without considering the issue of conformance, our testing needs significant improvement. It's time to raise the bar.

@jagosan
Copy link
Contributor

jagosan commented Feb 16, 2018

It's true that much of the 'gap' can be closed by promoting existing tests to conformance tests. The working group intentionally focused on creating a formal process for proposing/promoting new conformance tests rather than expanding coverage initially. This working doc proposes some guidelines for next efforts.

Not all tests should be in the conformance test suite, certainly, at least, for the 'base profile'. An explicit goal has been that features that require the capacity of a public cloud, e.g., are out of scope for the 'base profile' and there has not been significant effort invested in defining any additional profiles (though the mechanism is supported in the terms and conditions).

@liggitt
Copy link
Member

liggitt commented Feb 16, 2018

I didn't intend to get into the technical details of how to add conformance tests in this PR, much as it doesn't describe how to write other kinds of tests, either. We need other documents to provide answers to your questions.

That is perfectly valid, I was just concerned by the CNCF project proposal citing this policy as dovetailing with its specific technical goals, and that it would lead to development efforts improving metrics that may not actually matter.

We don’t have good coverage measurement.

I agree. The measurements used to plan improvements, judge success, and ensure future work doesn't regress coverage should be the first step.

But I don't think solving all of those problems should block the policy. We need to start chipping away at this.

In any case, even without considering the issue of conformance, our testing needs significant improvement. It's time to raise the bar.

I agree, and look forward to progress in this area.

@bgrant0607
Copy link
Member Author

Any other comments before we merge this?

@brahmaroutu
Copy link
Contributor

I have created a PR to add guidelines for writing comments on conformance tests for generating the conformance document. #1867
I am looking for your input.

First version of conformance document is here and we are working on better descriptions for the test.
@WilliamDenniss

@bgrant0607
Copy link
Member Author

I'm going to apply Clayton's lgtm for real now.

/lgtm

@k8s-ci-robot
Copy link
Contributor

@bgrant0607: you cannot LGTM your own PR.

In response to this:

I'm going to apply Clayton's lgtm for real now.

/lgtm

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.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bgrant0607

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

@bgrant0607 bgrant0607 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 1, 2018
@k8s-ci-robot k8s-ci-robot merged commit 8864f83 into kubernetes:master Mar 1, 2018
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. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants