Distinguish projected secret, configmap and downward api tests #68910

msau42 · 2018-09-20T23:34:25Z

What this PR does / why we need it:

Split up projected.go into multiple files: projected_secret/configmap/downwardapi/combined.go
Added separate Context() with secret, configmap and downward api for projected tests.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #68904

Special notes for your reviewer:

Release note:

NONE

msau42 · 2018-09-20T23:37:16Z

Ok, the diff looks ridiculous, but all I did was add 3 Context() blocks!

msau42 · 2018-09-20T23:37:30Z

/assign @BenTheElder

msau42 · 2018-09-20T23:38:14Z

test/conformance/testdata/conformance.txt

 test/e2e/common/projected.go: "should be consumable from pods in volume"
 test/e2e/common/projected.go: "should be consumable from pods in volume with defaultMode set"
 test/e2e/common/projected.go: "should be consumable from pods in volume as non-root"
 test/e2e/common/projected.go: "should be consumable from pods in volume with mappings"
 test/e2e/common/projected.go: "should be consumable from pods in volume with mappings and Item mode set"
 test/e2e/common/projected.go: "should be consumable from pods in volume with mappings as non-root"
 test/e2e/common/projected.go: "updates should be reflected in volume"
-test/e2e/common/projected.go: "optional updates should be reflected in volume"
+test/e2e/common/projected.go: "configMap with updates should be reflected in volume"


BenTheElder · 2018-09-21T00:02:23Z

Thank you! cc @spiffxp

BenTheElder · 2018-09-21T00:04:55Z

test/conformance/testdata/conformance.txt

@@ -112,15 +112,15 @@ test/e2e/common/projected.go: "should be consumable from pods in volume as non-r
 test/e2e/common/projected.go: "should be consumable from pods in volume with mappings"
 test/e2e/common/projected.go: "should be consumable from pods in volume with mappings and Item Mode set"
 test/e2e/common/projected.go: "should be consumable in multiple volumes in a pod"
-test/e2e/common/projected.go: "optional updates should be reflected in volume"
+test/e2e/common/projected.go: "updates should be reflected in volume"
 test/e2e/common/projected.go: "should be consumable from pods in volume"
 test/e2e/common/projected.go: "should be consumable from pods in volume with defaultMode set"
 test/e2e/common/projected.go: "should be consumable from pods in volume as non-root"
 test/e2e/common/projected.go: "should be consumable from pods in volume with mappings"


this one is still duplicated AFAICT (see line 112 currently)

I made the full test case name unique, by adding in another Context(). However, the string inside the It() is still duplicated.

Do we need the It() strings to be unique?

the other option I could do is to split out the tests into 3 different files: projected_secret, projected_configmap, projected_downwardapi

The full identifier needs to be unique, as long as that's the case. I think you're right and that this will already work but it might be worth splitting up anyhow?

BenTheElder · 2018-09-21T00:06:00Z

test/conformance/testdata/conformance.txt

@@ -112,15 +112,15 @@ test/e2e/common/projected.go: "should be consumable from pods in volume as non-r
 test/e2e/common/projected.go: "should be consumable from pods in volume with mappings"
 test/e2e/common/projected.go: "should be consumable from pods in volume with mappings and Item Mode set"
 test/e2e/common/projected.go: "should be consumable in multiple volumes in a pod"
-test/e2e/common/projected.go: "optional updates should be reflected in volume"
+test/e2e/common/projected.go: "updates should be reflected in volume"
 test/e2e/common/projected.go: "should be consumable from pods in volume"
 test/e2e/common/projected.go: "should be consumable from pods in volume with defaultMode set"


this one as well (see line 110 currently)

Ok after splitting them into separate files, I am still seeing some duplicates. So far it looks like at least one of the duplicate tests is a superset of the other. Can I remove conformance tests?

I'm not sure what the rules for removing conformance tests are, They're expected to not change post-release but I'm not sure about during a release.

@spiffxp might be able to comment on this.

er and yeah splitting the files wouldn't be enough I think, they'd need split-file-matching contexts I think? the generated [sig-storage] Projected should be consumable from pods in volume with mappings [NodeConformance] [Conformance] entry needs to be unique

I changed the top level Describe() of each new file to be unique

maybe we need to update the walker that generates this list, it strips out a lot

msau42 · 2018-09-21T00:51:59Z

test/e2e/common/projected_configmap.go

+	   Testname: Projected Volume, ConfigMap, update
+	   Description: A Pod is created with projected volume source ‘ConfigMap’ to store a configMap and performs a create and update to new value. Pod MUST be able to create the configMap with value-1. Pod MUST be able to update the value in the confgiMap to value-2.
+	*/
+	framework.ConformanceIt("updates should be reflected in volume [NodeConformance]", func() {


This test is a subset of the next test. I think we can remove this test.

@spiffxp is it ok if I remove this test from the conformance suite (and all suites in general)?

So optional and non optional projected volumes have exactly the same behavior?

Ah I didn't realize there was actually an Optional field in the API. I will add the test back. Maybe we should add one more test case to test pod failure when optional == false

One more observation: non-optional is not tested for secrets either. So some future areas to improve coverage:

non-optional for secrets

non-optional should fail for secrets when key doesn't exist

non-optional should fail for configmaps when key doesn't exist

spiffxp · 2018-09-21T01:38:20Z

/area conformance

spiffxp · 2018-09-21T01:48:22Z

/kind cleanup
/priority important-soon

spiffxp · 2018-09-21T18:19:38Z

/retest

msau42 · 2018-09-21T21:39:12Z

Updated and squashed:

Separated out tests into projected_configmap, projected_secret, projected_downwardapi
Removed one duplicate projected_configmap test

BenTheElder · 2018-09-21T21:47:57Z

Thanks so much for looking at this! Fixing this will make it viable to get ~correct results faster by running the conformance suite first in parallel for non-[Serial] and then in serial for the handful of serial tests.

msau42 · 2018-09-27T22:00:55Z

this is ready for review. PTAL @spiffxp

spiffxp · 2018-10-04T23:23:33Z

/milestone v1.13
/lgtm

…rdapi

msau42 · 2018-10-09T21:59:15Z

/test pull-kubernetes-integration

msau42 · 2018-10-09T22:01:15Z

Put the non-optional update configmap test case back.

So this PR is strictly:

split up projected.go file into each volume type
rename test suite with the specific volume type to avoid duplicate test case names

msau42 · 2018-10-09T22:25:07Z

/test pull-kubernetes-integration

spiffxp · 2018-10-09T22:27:54Z

/retest
... weird, why are there two different commit shas in those failure links

msau42 · 2018-10-09T23:44:08Z

/retest

msau42 · 2018-10-10T00:47:58Z

/retest

spiffxp · 2018-10-10T23:48:30Z

/retest

smarterclayton · 2018-10-12T17:03:10Z

/approve

k8s-ci-robot · 2018-10-12T17:03:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: msau42, 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:

~~test/OWNERS~~ [smarterclayton]
~~test/conformance/testdata/OWNERS~~ [smarterclayton]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

msau42 · 2018-10-13T00:44:28Z

ping @spiffxp for review again

spiffxp · 2018-10-13T01:10:38Z

/lgtm

k8s-ci-robot requested review from MrHohn and smarterclayton September 20, 2018 23:34

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 20, 2018

k8s-ci-robot assigned BenTheElder Sep 20, 2018

msau42 commented Sep 20, 2018

View reviewed changes

msau42 force-pushed the fix-names branch from 774d6bc to 9f146df Compare September 20, 2018 23:40

BenTheElder reviewed Sep 21, 2018

View reviewed changes

msau42 commented Sep 21, 2018

View reviewed changes

msau42 force-pushed the fix-names branch 2 times, most recently from 47775da to e4ed54f Compare September 21, 2018 01:07

k8s-ci-robot added the area/conformance Issues or PRs related to kubernetes conformance tests label Sep 21, 2018

msau42 force-pushed the fix-names branch from e4ed54f to 98dd5d8 Compare September 21, 2018 21:37

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 4, 2018

k8s-ci-robot added this to the v1.13 milestone Oct 4, 2018

msau42 force-pushed the fix-names branch from bac2464 to 6d7be53 Compare October 9, 2018 21:00

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Oct 9, 2018

msau42 added 4 commits October 9, 2018 14:25

Break up projected.go into projected_combined/configmap/secret/downwa…

f82ff5b

…rdapi

Keep related test cases and delete the rest

cb7cb9a

Update test suite names

253e8d5

Update build and conformance test list

17c0e9f

msau42 force-pushed the fix-names branch from 6d7be53 to 17c0e9f Compare October 9, 2018 21:28

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 9, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 12, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 13, 2018

k8s-ci-robot merged commit 79869af into kubernetes:master Oct 13, 2018

msau42 mentioned this pull request Oct 17, 2018

Add more projected volume e2e test cases #69911

Closed

pontiyaraja mentioned this pull request Oct 26, 2018

added non-optional and empty key test cases for secret and configMap #70299

Merged

msau42 deleted the fix-names branch November 17, 2018 21:08

Distinguish projected secret, configmap and downward api tests #68910

Distinguish projected secret, configmap and downward api tests #68910

Conversation

msau42 commented Sep 20, 2018 • edited

msau42 commented Sep 20, 2018

msau42 commented Sep 20, 2018

Choose a reason for hiding this comment

BenTheElder commented Sep 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spiffxp commented Sep 21, 2018

spiffxp commented Sep 21, 2018

spiffxp commented Sep 21, 2018

msau42 commented Sep 21, 2018

BenTheElder commented Sep 21, 2018

msau42 commented Sep 27, 2018

spiffxp commented Oct 4, 2018

msau42 commented Oct 9, 2018

msau42 commented Oct 9, 2018

msau42 commented Oct 9, 2018

spiffxp commented Oct 9, 2018

msau42 commented Oct 9, 2018

msau42 commented Oct 10, 2018

spiffxp commented Oct 10, 2018

smarterclayton commented Oct 12, 2018

k8s-ci-robot commented Oct 12, 2018

msau42 commented Oct 13, 2018

spiffxp commented Oct 13, 2018

msau42 commented Sep 20, 2018 •

edited