Moving envvars_test from e2e to conformance #2602

dushyanthsc · 2018-12-01T01:11:52Z

Change to update the EnvVars e2e test to include all environment variables specified in run-time contract.

Proposed Changes

Change e2e EnvVars test to include all environment variables specified in run-time contract.
Separate "MUST" and "SHOULD" environment variables to handle them separately
Update test/util to support server with path specific handlers.

knative-prow-robot · 2018-12-01T01:12:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dushyanthsc

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~~ [dushyanthsc]

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

knative-prow-robot

@dushyanthsc: 0 warnings.

In response to this:

Change to update the EnvVars e2e test to include all environment variables specified in run-time contract.

Proposed Changes

Change e2e EnvVars test to include all environment variables specified in run-time contract.

Separate "MUST" and "SHOULD" environment variables to handle them separately

Update test/util to support serve with path specific handlers.

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.

dushyanthsc · 2018-12-01T01:27:11Z

/assign @evankanderson @jessiezcc

test/e2e/envvars_test.go

jessiezcc · 2018-12-03T04:49:25Z

test/test_images/envvars/envvars.go

-	rev := os.Getenv("K_REVISION")
-	fmt.Fprintf(w, "Here are our env vars service:%s - configuration:%s - revision:%s", svc, cfg, rev)
+//Environment variables that "SHOULD" be set by knative implementation
+var shouldSet = []string{ "K_SERVICE", "K_CONFIGURATION", "K_REVISION"}


shouldSet and mustSet are defined in both test_image and tests, can they somehow share the code?

I did think of that, but I saw that OpenContainers use a different model where it looks like they do code-gen from spec. I was not sure if we would be moving to that model. Just as a layer of safety I have ensured that they match both in numbers and key-names, but I understand your point, will wait for Evan to comment to see whats the plan on having a single source of truth for specifying the expected keys.

I don't think we have a plan yet for auto-generating conformance tests based on the spec. If you have a specific idea or proposal, I'd be interested, but I don't have the bandwidth to work on that in the next ~year.

Got it. For this change taking into consideration your comment on a more permissive parsing model I have moved environment variables set to conformance/runtime_contact_types.go I will take a deeper look at how things are done in open-container and see if we can apply that or come up with a different approach as an action item.

jessiezcc · 2018-12-03T04:55:24Z

test/e2e/envvars_test.go

+	}
+
+	expectedOutputs = map[string]string{
+		"PORT" : "8080",


from conformance perspective, my understanding is that as long as PORT is set, it's conformant, doesn't have to be 8080.

The contract says "Ingress containerPort for ingress requests and health checks" and in the test image we create the service exposing ":8080" hence the check verifies the port number as well.

Can you add a comment explaining why this MUST be 8080 (i.e. "8080 taken from ..., if you change that, this line also needs to change" or the like)?

Suggestion: maybe use a different port from 8080, to catch cases where the port has been hard-coded to 8080?

I have moved the port value to be to a single file and referenced them in both test-image and the test. Also added comment explaining the value used for verification.

Regarding you suggestion, this issue: #2258 prohibits from using a different port. I have added a TODO in the code to change it when the above issue gets fixed.

evankanderson · 2018-12-03T19:08:39Z

test/e2e/envvars_test.go


-	envVarsExpectedOutput := fmt.Sprintf("Here are our env vars service:%s - configuration:%s - revision:%s", names.Service, names.Config, names.Revision)
+	domain := route.Status.Domain + "/envvars/should"


It's strange to call this "domain" -- maybe "url"?

My bad, I re-used name from earlier code, I have fixed it now.

evankanderson · 2018-12-03T19:11:19Z

test/e2e/envvars_test.go

+	}
+
+	expectedOutputs = map[string]string{
+		"PORT" : "8080",


Can you add a comment explaining why this MUST be 8080 (i.e. "8080 taken from ..., if you change that, this line also needs to change" or the like)?

Suggestion: maybe use a different port from 8080, to catch cases where the port has been hard-coded to 8080?

evankanderson · 2018-12-03T19:11:35Z

test/e2e/envvars_test.go

+	expectedOutputs = map[string]string{
+		"PORT" : "8080",
+	}
+	if err = 	verifyResponse(resp, "MUST", expectedOutputs); err != nil {


strange indent here. Run gofmt?

evankanderson · 2018-12-03T19:20:42Z

test/e2e/envvars_test.go

+		return errors.Errorf("Json Un-marshalling error : %v", jsonErr)
+	}
+
+	if len(expectedOutputs) != len(respMap) {


I'd tend to lean towards a more permissive parsing model here. In particular, this test ends up spreading the should/must logic across both the test itself and the running container. It might be simpler to do the following:

Have the target container simply return a JSON-encoded envvars map (object).

Have the test decode the JSON with a "should" and a "must" struct, like so:

var should struct { service string `json:"K_SERVICE"` configuration string `json:"K_CONFIGURATION"` revision string `json:"K_REVISION"` } var must struct { port string `json:"PORT"` } if err := json.Unmarshal(payload, &should); err != nil { ... } if err := json.Unmarshal(payload, &must); err != nil { ... } if should != struct{...} { ... } if must != struct{...} { ... }

Makes sense, I have updated the verification to use this model.

evankanderson · 2018-12-03T19:36:53Z

test/test_images/envvars/README.md


-When called, the server emits a message containing the values of the environment variables for knative service, configuraiton and revision.
+'must' and 'should' refer to the keywords "MUST" and "SHOULD" used in the Knative [run-time contract](/docs/runtime-contract.md) and provides environment variables that "MUST" and "SHOULD" be set by a Knative implementations


I'd prefer more generic interfaces on our test containers (e.g. one container that provides /envvars, /grow, /headers, etc) rather than having a bunch of purpose-built containers, one for each test.

This would allow better re-use of the container code where we didn't need to write a new container for each new test.

Makes sense, I have updated the test-image to be an environment test image that can be used to retrieve information about the environment in which the container runs. The server now exposes "/envvars" to fetch all the environment variables defined inside the container.

evankanderson · 2018-12-03T19:37:40Z

test/test_images/envvars/envvars.go

-	rev := os.Getenv("K_REVISION")
-	fmt.Fprintf(w, "Here are our env vars service:%s - configuration:%s - revision:%s", svc, cfg, rev)
+//Environment variables that "SHOULD" be set by knative implementation
+var shouldSet = []string{ "K_SERVICE", "K_CONFIGURATION", "K_REVISION"}


I don't think we have a plan yet for auto-generating conformance tests based on the spec. If you have a specific idea or proposal, I'd be interested, but I don't have the bandwidth to work on that in the next ~year.

dushyanthsc · 2018-12-05T23:13:05Z

/retest

dushyanthsc · 2018-12-06T00:57:27Z

/retest

dushyanthsc · 2018-12-06T19:14:44Z

/retest

dushyanthsc · 2018-12-06T22:37:57Z

/retest

Change moves the envvars_test.go from e2e to conformance folder to use the test as a conformance test for the environment variables requirement specified in the run-time contract (https://github.com/knative/serving/blob/master/docs/runtime-contract.md) The test image is also changed to be more generic and provide a server that can be used to get information about the environment under which the container runs. Currently the iamge only exposes environment variables defined inside the container by exposing "/envvars" to fetch all the environment variables. The image folder name is also changed to "environment" to indicate this semantic change. Change also adds test/conformance/constants.go to define a way to have constants that can be shared between test-images and tests. Change also adds test/conformance/runtime_contract_types.go to define the runtime contract as go types.

dushyanthsc · 2018-12-07T23:12:31Z

/retest

dushyanthsc · 2018-12-07T23:50:03Z

/retest

jessiezcc · 2018-12-12T18:43:50Z

test/conformance/envvars_test.go

+func TestMustEnvVars(t *testing.T) {
+	logger := logging.GetContextLogger("TestMustEnvVars")
+	var names test.ResourceNames
+	resp, err := fetchEnvInfo(t, logger, test.EnvImageEnvVarsPath, &names)


this value of resp is the same as what's in TestShouldEnvVars?

The response from the environment test image is retrieving all the environment variables defined inside the container. And the type of object used for json.Unmarshall is determining what are environment variables of interest, in this case type MustEnvvars. The advantage of this is the next time we add/delete any environment variable from the list of SHOULD or MUST we would only have to change it in one place which is runtime_contract_types.go and everything else would just work.

jessiezcc · 2018-12-12T18:54:26Z

test/conformance/envvars_test.go

+)
+
+//TestShouldEnvVars verifies environment variables that are declared as "SHOULD be set" in runtime-contract
+func TestShouldEnvVars(t *testing.T) {


from code, I see that we validate Must and Should in a very similar way, i.e. check whether they are specified. I could be missing something. Can you call out the validation difference in comments so that it's obvious?

The reason for breaking them into two tests is to adhere to the semantics in the runtime contract as they might have different policies (like SHOULD generating warnings and MUST generating failures). As this is still to be decided, the PR currently just breaks them into two tests and fails on both instances.

jessiezcc · 2018-12-12T19:03:18Z

chatted with @dushyanthsc offline on review comments.
/lgtm

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 1, 2018

knative-prow-robot requested review from srinivashegde86 and steuhs December 1, 2018 01:11

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 1, 2018

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 1, 2018

knative-prow-robot reviewed Dec 1, 2018

View reviewed changes

knative-prow-robot assigned evankanderson and jessiezcc Dec 1, 2018

jessiezcc reviewed Dec 3, 2018

View reviewed changes

test/e2e/envvars_test.go Outdated Show resolved Hide resolved

jessiezcc reviewed Dec 3, 2018

View reviewed changes

dushyanthsc force-pushed the e2e/envvar-update branch from 873a18f to 19640f8 Compare December 3, 2018 18:52

evankanderson reviewed Dec 3, 2018

View reviewed changes

dushyanthsc changed the title ~~WIP: EnvVars e2e test modification~~ Moving envvars_test from e2e to conformance Dec 5, 2018

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 5, 2018

dushyanthsc force-pushed the e2e/envvar-update branch from b925d5a to d4691ad Compare December 5, 2018 22:37

dushyanthsc force-pushed the e2e/envvar-update branch from d4691ad to 9ddaa86 Compare December 5, 2018 23:44

dushyanthsc force-pushed the e2e/envvar-update branch 2 times, most recently from d50656e to 8f4925d Compare December 6, 2018 17:42

dushyanthsc force-pushed the e2e/envvar-update branch from 8f4925d to c6fecda Compare December 6, 2018 19:40

dushyanthsc force-pushed the e2e/envvar-update branch 2 times, most recently from 4148168 to cbe48b2 Compare December 7, 2018 19:02

dushyanthsc force-pushed the e2e/envvar-update branch from cbe48b2 to ef379bc Compare December 7, 2018 22:22

jessiezcc reviewed Dec 12, 2018

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 12, 2018

knative-prow-robot merged commit d525d17 into knative:master Dec 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moving envvars_test from e2e to conformance #2602

Moving envvars_test from e2e to conformance #2602

dushyanthsc commented Dec 1, 2018 •

edited

Loading

knative-prow-robot commented Dec 1, 2018

knative-prow-robot left a comment

dushyanthsc commented Dec 1, 2018

jessiezcc Dec 3, 2018

dushyanthsc Dec 3, 2018 •

edited

Loading

evankanderson Dec 3, 2018

dushyanthsc Dec 5, 2018

jessiezcc Dec 3, 2018

dushyanthsc Dec 3, 2018

evankanderson Dec 3, 2018

dushyanthsc Dec 5, 2018

evankanderson Dec 3, 2018

dushyanthsc Dec 5, 2018

evankanderson Dec 3, 2018

evankanderson Dec 3, 2018

dushyanthsc Dec 5, 2018

evankanderson Dec 3, 2018

dushyanthsc Dec 5, 2018

evankanderson Dec 3, 2018

dushyanthsc Dec 5, 2018

evankanderson Dec 3, 2018

dushyanthsc commented Dec 5, 2018

dushyanthsc commented Dec 6, 2018

dushyanthsc commented Dec 6, 2018

dushyanthsc commented Dec 6, 2018

dushyanthsc commented Dec 7, 2018

dushyanthsc commented Dec 7, 2018

jessiezcc Dec 12, 2018 •

edited

Loading

dushyanthsc Dec 12, 2018

jessiezcc Dec 12, 2018

dushyanthsc Dec 12, 2018

jessiezcc commented Dec 12, 2018


		envVarsExpectedOutput := fmt.Sprintf("Here are our env vars service:%s - configuration:%s - revision:%s", names.Service, names.Config, names.Revision)
		domain := route.Status.Domain + "/envvars/should"


		When called, the server emits a message containing the values of the environment variables for knative service, configuraiton and revision.
		'must' and 'should' refer to the keywords "MUST" and "SHOULD" used in the Knative [run-time contract](/docs/runtime-contract.md) and provides environment variables that "MUST" and "SHOULD" be set by a Knative implementations

Moving envvars_test from e2e to conformance #2602

Moving envvars_test from e2e to conformance #2602

Conversation

dushyanthsc commented Dec 1, 2018 • edited Loading

Proposed Changes

knative-prow-robot commented Dec 1, 2018

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

dushyanthsc commented Dec 1, 2018

Choose a reason for hiding this comment

dushyanthsc Dec 3, 2018 • edited Loading

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

Choose a reason for hiding this comment

dushyanthsc commented Dec 5, 2018

dushyanthsc commented Dec 6, 2018

dushyanthsc commented Dec 6, 2018

dushyanthsc commented Dec 6, 2018

dushyanthsc commented Dec 7, 2018

dushyanthsc commented Dec 7, 2018

jessiezcc Dec 12, 2018 • edited Loading

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

jessiezcc commented Dec 12, 2018

dushyanthsc commented Dec 1, 2018 •

edited

Loading

dushyanthsc Dec 3, 2018 •

edited

Loading

jessiezcc Dec 12, 2018 •

edited

Loading