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 check-conformance-test-requirements.go #75524

Merged
merged 1 commit into from Apr 15, 2019

Conversation

@oomichi
Copy link
Member

commented Mar 20, 2019

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind bug

What this PR does / why we need it:

We have defined requirements of conformance test as 1, and this
adds coding check for one requirement "it works for all providers".

If a conformance test contains SkipIfProviderIs() like

$ git diff
diff --git a/test/e2e/kubectl/kubectl.go b/test/e2e/kubectl/kubectl.go
index 3f197b2b64..0a1c562339 100644
--- a/test/e2e/kubectl/kubectl.go
+++ b/test/e2e/kubectl/kubectl.go
@@ -274,7 +274,7 @@ var _ = SIGDescribe("Kubectl client", func() {
                */
                framework.ConformanceIt("should create and stop a replication controller ", func() {
                        defer cleanupKubectlInputs(nautilus, ns, updateDemoSelector)
-
+                       framework.SkipIfProviderIs("aws")
                        ginkgo.By("creating a replication controller")
                        framework.RunKubectlOrDieInput(nautilus, "create", "-f", "-", fmt.Sprintf("--namespace=%v", ns))
                        framework.ValidateController(c, nautilusImage, 2, "update-demo", updateDemoSelector, getUDData("nautilus.jpg", ns), ns)

this script detects like

test/e2e/kubectl/kubectl.go: Conformance test should not call SkipIfProviderIs()/SkipUnlessProviderIs()
Error: We need to fix the above errors.
exit status 1

Which issue(s) this PR fixes:

Fixes #74432

Does this PR introduce a user-facing change?:

NONE
@oomichi

This comment has been minimized.

Copy link
Member Author

commented Mar 20, 2019

/assign @bgrant0607

@timothysc

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

/hold

give that a majority of the codebase is bash or go I'd recommend refactoring to either of those 2.

@oomichi oomichi force-pushed the oomichi:issue/74432 branch from 609fcac to ac09b5e Mar 21, 2019

@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Mar 21, 2019

@oomichi oomichi changed the title Add check-conformance-test-requirements.pl Add check-conformance-test-requirements.go Mar 21, 2019

@timothysc
Copy link
Member

left a comment

So most of the other applications in k8s follow a similar pattern or style that is k8s-idiomatic-ish go. I'd recommend follow the pattern of other cli under the cmd directory.

Also, did you verify that this properly hooks into all the other check functionality that are run per PR?

if inConformanceCode {
if regSkipProviderIs.MatchString(line) {
fmt.Printf("%v: Conformance test should not call SkipIfProviderIs()/SkipUnlessProviderIs()\n", e2eFile)
exitCode = 1

This comment has been minimized.

Copy link
@timothysc

timothysc Mar 21, 2019

Member

You could just return error from this function and any error = fail.

This comment has been minimized.

Copy link
@oomichi

oomichi Mar 21, 2019

Author Member

I was thinking it would be nice to list all invalid places on one operation.


func main() {
flag.Parse()
args := flag.Args()

This comment has been minimized.

Copy link
@timothysc

timothysc Mar 21, 2019

Member

You should probably declare input arguments either with pflags or cobra.

This comment has been minimized.

Copy link
@oomichi

oomichi Mar 21, 2019

Author Member

cobra seems majority under cmd. I will try it with cobra.

inConformanceCode := false

//e.g. framework.ConformanceIt("should provide secure master service ", func() {
regStartConformance := regexp.MustCompile("framework.ConformanceIt\\(.*, func\\(\\) {$")

This comment has been minimized.

Copy link
@timothysc

timothysc Mar 21, 2019

Member

most of these strings can be consts above.

This comment has been minimized.

Copy link
@oomichi

oomichi Mar 21, 2019

Author Member

Thanks done

line := scanner.Text()
if regStartConformance.MatchString(line) {
if inConformanceCode {
panic("Missed the end of previous conformance test. There might be a bug in this script.")

This comment has been minimized.

Copy link
@timothysc

timothysc Mar 21, 2019

Member

could just direct return with an error

This comment has been minimized.

Copy link
@oomichi

oomichi Mar 21, 2019

Author Member

OK, done

}
}
if inConformanceCode {
panic("Missed the end of previous conformance test. There might be a bug in this script.")

This comment has been minimized.

Copy link
@timothysc

timothysc Mar 21, 2019

Member

same comments with panic.

This comment has been minimized.

Copy link
@oomichi

oomichi Mar 21, 2019

Author Member

done

@timothysc

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

/assign @timothysc

@oomichi

This comment has been minimized.

Copy link
Member Author

commented Mar 21, 2019

@timothysc

Thanks for your review.

So most of the other applications in k8s follow a similar pattern or style that is k8s-idiomatic-ish go.
I'd recommend follow the pattern of other cli under the cmd directory.

OK, will check that and make this follow it.

Also, did you verify that this properly hooks into all the other check functionality that are run per PR?

Does "all the other check functionality" mean the other requirements of https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/conformance-tests.md#conformance-test-requirements ?

If so, I'd like to make this small as possible like current one for making a consensus.
(That is good for me because I've avoided much translation today)

@oomichi oomichi changed the title Add check-conformance-test-requirements.go WIP: Add check-conformance-test-requirements.go Mar 21, 2019

@oomichi oomichi force-pushed the oomichi:issue/74432 branch 2 times, most recently from 127cf31 to 5839c92 Mar 21, 2019

@oomichi

This comment has been minimized.

Copy link
Member Author

commented Mar 21, 2019

/retest

@oomichi

This comment has been minimized.

Copy link
Member Author

commented Mar 21, 2019

@timothysc Thanks for your review. I did mistakes on previous one and have updated it based on your comments. Could you take a look again?

@timothysc
Copy link
Member

left a comment

Minor comments.

Still not a fan of the global on exit code.


fileInput, err := ioutil.ReadFile(e2eFile)
if err != nil {
fmt.Fprintf(os.Stderr, "Failed to read file %s: %v\n", e2eFile, err)

This comment has been minimized.

Copy link
@timothysc

timothysc Mar 22, 2019

Member

you could use errors.Wrap here and just output on the calling function.

This comment has been minimized.

Copy link
@oomichi

oomichi Mar 22, 2019

Author Member

Thanks, I've learn that is useful today.
Done


files, err := ioutil.ReadDir(e2ePath)
if err != nil {
fmt.Fprintf(os.Stderr, "Failed to read dir %s: %v\n", e2ePath, err)

This comment has been minimized.

Copy link
@timothysc

timothysc Mar 22, 2019

Member

same comment about wrapping the error and printing on the caller.

This comment has been minimized.

Copy link
@oomichi

oomichi Mar 22, 2019

Author Member

done

Add check-conformance-test-requirements.go
We have defined requirements of conformance test as [1], and this
adds coding check for one requirement "it works for all providers".

[1]: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/conformance-tests.md#conformance-test-requirements
@oomichi

This comment has been minimized.

Copy link
Member Author

commented Mar 22, 2019

@timothysc

Still not a fan of the global on exit code.

OK, let me try remove the global exit code.

@oomichi oomichi force-pushed the oomichi:issue/74432 branch from e77f640 to 3d64667 Mar 22, 2019

@spiffxp spiffxp added this to To Triage in cncf-k8s-conformance-wg Mar 26, 2019

@oomichi

This comment has been minimized.

Copy link
Member Author

commented Mar 30, 2019

@timothysc Hi, I have applied your comments on the latest PR.
Could you take a look at this again?

@johnbelamaric johnbelamaric moved this from To Triage to In Progress in cncf-k8s-conformance-wg Apr 1, 2019

@johnbelamaric johnbelamaric moved this from In Progress to To Triage in cncf-k8s-conformance-wg Apr 1, 2019

@johnbelamaric johnbelamaric moved this from To Triage to In Review in cncf-k8s-conformance-wg Apr 1, 2019

@hh

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

@timothysc I'd like to review this as well.

@timothysc

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

/assign @hh

@timothysc
Copy link
Member

left a comment

/lgtm

can needle, but calling it good enough for now and once we turn it on for verifying, I'm sure we'll find small things.

@k8s-ci-robot k8s-ci-robot added the lgtm label Apr 11, 2019

@timothysc timothysc added this to the v1.15 milestone Apr 11, 2019

@timothysc timothysc moved this from In Review to Needs Approval in cncf-k8s-conformance-wg Apr 11, 2019

@smarterclayton

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: oomichi, smarterclayton

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

@oomichi

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2019

/hold cancel

@fejta-bot

This comment has been minimized.

Copy link

commented Apr 15, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

1 similar comment
@fejta-bot

This comment has been minimized.

Copy link

commented Apr 15, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit 4a5151b into kubernetes:master Apr 15, 2019

20 checks passed

cla/linuxfoundation oomichi authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Job succeeded.
Details
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Context retired. Status moved to "pull-kubernetes-dependencies".
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

cncf-k8s-conformance-wg automation moved this from Needs Approval to Done Apr 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.