How do we manually verify that the expected value in the unittests is correct? #306

jlewi · 2019-08-23T17:52:41Z

How are we supposed to verify that the expected values used by unittests is correct?

As an example: kubeflow/kubeflow#3986 appears to be due to a bug in the kustomize manifest resulting in environment variables not being set correctly to the variable value; instead they remained set to the variable name e.g. `$(userid-prefix)

I would expect this is something we test in the unittest.

So my expectation would be that I can look at the unittest and verify by inspection that the expected value is what it should be.

But when I look at the unittest

manifests/tests/centraldashboard-base_test.go

Line 76 in 76bd875

value: $(userid-header)

It looks like we are specifying the kustomize files (i.e. the input) and not the expected output.
For example here:

manifests/tests/jupyter-web-app-base_test.go

Line 242 in 76bd875

imagePullPolicy: $(policy)

We are setting

        image: gcr.io/kubeflow-images-public/jupyter-web-app:v0.5.0
        imagePullPolicy: $(policy)

The expected value for imagePullPolicy should not be $(policy). So either that is the input value and not the expected value or else our tests are wrong.

Can someone familiar with the unittest infrastructure help educate me?

My expectation is that unittests would work as follows

A golden set of manifests for different scenarios would be checked in somewhere (either as go code or as separate files in a "golden") directory
- Thus I can look at those golden files to see if the expected value is what it should be
The tests would be comparing the generated files to the golden files.
There might be some scripts or utilities to regenerate the golden files (e.g. by running kustomize)
- During code review it should be easy to inspect the diff to the golden files to allow manual verification of the new golden data

/cc @johnugeorge @yanniszark @kkasravi

The text was updated successfully, but these errors were encountered:

jlewi · 2019-08-23T18:05:18Z

What is this code doing?

manifests/tests/jupyter-web-app-base_test.go

Line 451 in 76bd875

th.assertActualEqualsExpected(m, string(expected))

Looks like we are running kustomize to get the expected value.

kkasravi · 2019-08-23T19:30:50Z

if values aren't replaced and one or more warnings are emitted

2019/08/23 12:29:59 well-defined vars that were never replaced: clusterDomain

we should FAIL on that unit test.

This is slightly complicated by the fact that we produce and run golang tests under overlays.
I think the best approach would be to

call kfctl's generate so that the top level kustomization.yamls are produced
the go unit test should be based on this kustomization.yaml
generate should be called for all supported config files
any non-resolved vars should result in a FAIL with failure indicating the config file and var
the resulting yaml stream should be piped to kubectl apply --dry-run --validate -f -

The unit tests are based on kustomize's unit test framework kusttestharness

kkasravi · 2019-08-23T19:45:17Z

@jlewi I've opened #308 PTAL

jlewi · 2019-08-24T20:30:18Z

thanks @kkasravi

generate should be called for all supported config files

What is a support config file in this case? Do you mean all support KfDef YAML files?

All of your comments sound like sound improvements. I'm still not clear how we would ensure that the expected values are correct.

How do we fix the unittests so that its easy to verify by manual inspection that the expected values are correct?

kkasravi · 2019-08-25T00:02:30Z

As far as vars go - it would be easy to grep for '$(' values

jlewi · 2019-08-25T21:57:06Z

How does grepping for "$(" allow me to verify that the expected output is correct?

Shouldn't the unittests be doing something like taking a kustomization.yaml, params.env as input running kustomize build and then ensuring the generated output matches some expected golden output?

Here's the deployment for the profiles controller
https://github.com/kubeflow/manifests/blob/master/profiles/base/deployment.yaml

It has several variables e.g. $(userid-header) in the command line argument.

So my expectation is that using the unittest I can verify that kustomize is correctly substituting in the correct values for those variables and generating the appropriate command line argument.

Looking at the unittest
https://github.com/kubeflow/manifests/blob/master/tests/profiles-base_test.go

How can I verify that the expected value that the unittest is testing against is what I expect? I'd like to be able to inspect the unittest and verify that the unittest is using an expected value that ensures the parameters are correctly substituted. I'd expect that by looking at the unittest I could see that the input was params.env with $(userid-header) set to some value e.g. "fake-header" and that the expected value for the deployment had a command line like

 args:
       ....
        - "-userid-header"
        - fake-header

kkasravi · 2019-08-25T22:35:01Z

so some kind of output like

admin=xxxx
userid-header=yyyy
userid-prefix=zzzz

and

apiVersion: apps/v1
kind: Deployment
metadata:
  name: deployment
spec:
  template:
    spec:
      containers:
      - command:
        - /manager
        args:
        - "-userid-header"
        - yyyy
        - "-userid-prefix"
        - zzzz
        image: gcr.io/kubeflow-images-public/profile-controller:v20190619-v0-219-gbd3daa8c-dirty-1ced0e
        imagePullPolicy: Always
        name: manager
      - command:
        - /opt/kubeflow/access-management
        args:
        - "-cluster-admin"
        - xxxx
        - "-userid-header"
        - yyyy
        - "-userid-prefix"
        - zzzz
        image: gcr.io/kubeflow-images-public/kfam:v20190612-v0-170-ga06cdb79-dirty-a33ee4
        imagePullPolicy: Always
        name: kfam
      serviceAccountName: controller-service-account

with the substitutions in red or something?

jlewi · 2019-08-25T22:54:22Z

What does in read mean? Can we follow the normal structure of go uniitests?

The normal structure of a go unittest would be to define a testCase struct that captures input and expected output. So in our case that might be something like the following

type testCase struct {
  kustomization ...
  params map[string]string
  kustomizeAppDir string
  expectedYamlDir 
}

So kustomization would be the input to a kustomization file and params would be a list of the parameters, and kustomizeAppDir would be the directory of the application manifest. (We could add other inputs as necessary).

expectedYamlDir would be a directory (or file) where I could find the fully evaluated YAML representing the expected output of kustomize build.

So now to verify that the expected output is correct I can just look at expectedYamlDir. So that's where I'd expect to see the Deployment spec you listed above.

So now if the test compares the output of kustomize build to that expectedYamlDir and the test passes then I know my kustomize files will produce the expected output (i.e. that the expected substitutions take place).

Right now its not clear to me what the unittests are testing. I'm guessing they are just verifying that if you run kustomize you generate valid K8s specs.

kkasravi · 2019-08-26T00:47:13Z

ok, I'll implement in #308

jlewi · 2019-08-27T14:02:38Z

Thanks @kkasravi

Repasting your [comment] (#296 (comment))

Each unit test embeds the content of current files in the unit test - for example
profiles-base_test.go contains the following files

$ ☞  grep write[FK] profiles-base_test.go
	th.writeF("/manifests/profiles/base/crd.yaml", `
	th.writeF("/manifests/profiles/base/service-account.yaml", `
	th.writeF("/manifests/profiles/base/cluster-role-binding.yaml", `
	th.writeF("/manifests/profiles/base/role.yaml", `
	th.writeF("/manifests/profiles/base/role-binding.yaml", `
	th.writeF("/manifests/profiles/base/service.yaml", `
	th.writeF("/manifests/profiles/base/deployment.yaml", `
	th.writeF("/manifests/profiles/base/params.yaml", `
	th.writeF("/manifests/profiles/base/params.env", `
	th.writeK("/manifests/profiles/base", `

this is run through kustomize build and the current files are run through kustomize build. Then both are EncodeAsYaml and compared.

If the PR author makes changes and doesn't run generate, then failure is guaranteed. In order to update the unit tests with their changes, they should run generate. Now the unit test has embedded their changes and expected should equal actual.

So IIUC; the way the unittests are currently designed is we take the kustomize packages at commit N and commit N-1; run kustomize build for both and then compare the output?

kkasravi · 2019-08-27T16:07:44Z

correct. However the actual vs expected were reversed - see PR #325

jlewi · 2019-12-13T02:34:53Z

I'm still not sure how I'm supposed to verify that the expected output is correct. It looks like make generate is creating a kustomize package

manifests/tests/application-base_test.go

Line 91 in 67eabbf

value: $(project)

And not generating the expected output of kustomize.

See also @krishnadurai 's comment

#665 (comment)

I think my expectation is that

make generate recurses through the manifests directory and runs kustomize build to generate golden set of files
Tests run kustomize build and compare against the golden files

So a reviewer is checking the following

Looks at the diff of golden files to see if expected output of kustomize build is correct
Ensures tests pass

It looks like gen-test-target.sh might be generating parts of the kustomize package. Is there any reason we can't create suitable kustomization.yaml files such that kustomize build generates suitable output?

krishnadurai · 2019-12-13T19:41:31Z

One of the ways of generating kustomize output is by using '-o' flag as suggested by the this PR: kubernetes-sigs/kustomize#960. This generates individual output files and can dump them into a folder.

I suggest we could use this kustomize feature in gen-test-target.sh instead of trying to interpret kustomization.yaml.

This will allow us to maintain a set of golden output files and compare against them.

I'll take a stab at implementing this.

jtfogarty · 2020-01-06T21:51:54Z

/kind feature

jlewi · 2020-03-15T22:32:39Z

@krishnadurai any update on this? I think I'm going to need these changes to the test infra before I can start refactoring the manifests (e.g. #962) to start removing some of the kfctl magic (#774)

jlewi · 2020-03-16T00:14:59Z

I created a one off example of what this might look like here:
https://github.com/jlewi/manifests/blob/stacks/tests/jupyter/jupyter-web-app/base_v3/jupyter-jupyter-web-app-base_v3_test.go

Next steps would be

Update generate_tests.py to generate YAML files in appropriate directory by running kustomize build -o
Generate ${PACKAGE}_test.go files for all manifests

krishnadurai · 2020-03-16T09:51:21Z

/assign

jlewi · 2020-03-16T14:12:46Z

@krishnadurai I think I almost have something working in https://github.com/jlewi/manifests/tree/stacks
which is ready for a PR.

I've re-written generate.py to run kustomize build
- https://github.com/jlewi/manifests/blob/stacks/hack/generate_tests.py
I've created some new test utilities test_util.go
- To run the tests

How does this compare to what you have or were planning on working on? What's the best way to combine efforts? Want me to split out these changes into their own branch/PR?

krishnadurai · 2020-03-16T15:01:58Z

@jlewi I was planning to pick this up and change https://github.com/kubeflow/manifests/blob/master/hack/gen-test-target.sh to a python script.
Though it seems that you have already made the changes splitting resources in an output directory with kustomize build -o, which was what I was looking to contribute.

I'll be looking forward to reviewing your work when it is ready.

Want me to split out these changes into their own branch/PR?

I'm fine with a single PR.

/unassign

…AML resources. * Per kubeflow#306 to allow reviewers to verify that the expected output is correct we should check in the result of "kustomize build -o" so that reviewers can review the diff and verify that it is correct. * This also simplifies the test generation code; the python script generate_tests.py just recourses over the directory tree and runs "kustomize build -o" and checks in the output into the test_data directory. * This is different from what the tests are currently doing. * Currently what the generation scripts do is generate "kustomization.yaml" files and then generate the expected output from that when the test is run. * This makes it very difficult to validate the expected output and to debug whether the expected output is correct. * Going forward, per kubeflow#1014, I think what we want to do is check in test cases corresponding to kustomization.yaml files corresponding to various kustomizations that we want to validate are working correctly * Our generate scripts would then run "kustomize build" to generate expected output and check that in so that we can validate that the expected output is correct. * Also change the tests data structure so that it mirrors the kustomize directory tree rather than flattening the tests into the "tests" directory. * Fix kubeflow#683 * Right now running the unittests takes a long time * The problem is that we generate unittests for every "kustomization.yaml" file * Per kubeflow#1014 this is kind of pointless/redundant because most of these tests aren't actually testing kustomizations. * We will address this in follow on PRs which will add more appropriate tests and remove some of these unnecessary/redundant tests.

…AML resources. (#1016) * unittests should compare result of kustomize build to golden set of YAML resources. * Per #306 to allow reviewers to verify that the expected output is correct we should check in the result of "kustomize build -o" so that reviewers can review the diff and verify that it is correct. * This also simplifies the test generation code; the python script generate_tests.py just recourses over the directory tree and runs "kustomize build -o" and checks in the output into the test_data directory. * This is different from what the tests are currently doing. * Currently what the generation scripts do is generate "kustomization.yaml" files and then generate the expected output from that when the test is run. * This makes it very difficult to validate the expected output and to debug whether the expected output is correct. * Going forward, per #1014, I think what we want to do is check in test cases corresponding to kustomization.yaml files corresponding to various kustomizations that we want to validate are working correctly * Our generate scripts would then run "kustomize build" to generate expected output and check that in so that we can validate that the expected output is correct. * Also change the tests data structure so that it mirrors the kustomize directory tree rather than flattening the tests into the "tests" directory. * Fix #683 * Right now running the unittests takes a long time * The problem is that we generate unittests for every "kustomization.yaml" file * Per #1014 this is kind of pointless/redundant because most of these tests aren't actually testing kustomizations. * We will address this in follow on PRs which will add more appropriate tests and remove some of these unnecessary/redundant tests. * Cherry pick AWS fixes. * Regenerate the tests. * Fix the unittests; need to update the generate logic to remove unused tests to remove tests that aren't part of this PR. * Address comments. * Rebase on master and regenerate the tests.

jlewi · 2020-03-25T03:54:16Z

#1016 has been merged so closing this.

jlewi added priority/p1 area/kfctl labels Aug 23, 2019

This was referenced Sep 10, 2019

Unit tests in v0.6-branch are not up-to-date and updating them makes tests fail #296

Closed

fix for backtick escape error #358

Merged

jlewi mentioned this issue Sep 19, 2019

To support uniform and unique configmaps names across kubeflow applications #331

Closed

1 task

This was referenced Oct 3, 2019

kubeflow/manifests @ master is broken - uber issue #426

Closed

just add application overlays and generateName to current application… #435

Closed

jlewi mentioned this issue Dec 13, 2019

Fix computation of changed files in generate-changed-only rule #665

Merged

jlewi mentioned this issue Dec 17, 2019

manifests test structure should mirror manifest directory tree #683

Closed

k8s-ci-robot added the kind/feature label Jan 6, 2020

jlewi added area/kustomize and removed area/kfctl labels Mar 16, 2020

k8s-ci-robot assigned krishnadurai Mar 16, 2020

k8s-ci-robot unassigned krishnadurai Mar 16, 2020

This was referenced Mar 16, 2020

Use a configmap generator for the spawner_ui_config.yaml #1013

Merged

unittests should test kustomizations #1014

Closed

unittests should validate Kubeflow patterns #1015

Closed

jlewi mentioned this issue Mar 16, 2020

unittests should compare result of kustomize build to golden set of YAML resources. #1016

Merged

jlewi closed this as completed Mar 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

How do we manually verify that the expected value in the unittests is correct? #306

How do we manually verify that the expected value in the unittests is correct? #306

jlewi commented Aug 23, 2019

jlewi commented Aug 23, 2019

kkasravi commented Aug 23, 2019 •

edited

Loading

kkasravi commented Aug 23, 2019

jlewi commented Aug 24, 2019

kkasravi commented Aug 25, 2019

jlewi commented Aug 25, 2019

kkasravi commented Aug 25, 2019

jlewi commented Aug 25, 2019

kkasravi commented Aug 26, 2019

jlewi commented Aug 27, 2019

kkasravi commented Aug 27, 2019

jlewi commented Dec 13, 2019

krishnadurai commented Dec 13, 2019

jtfogarty commented Jan 6, 2020

jlewi commented Mar 15, 2020

jlewi commented Mar 16, 2020

krishnadurai commented Mar 16, 2020

jlewi commented Mar 16, 2020

krishnadurai commented Mar 16, 2020 •

edited

Loading

jlewi commented Mar 25, 2020

How do we manually verify that the expected value in the unittests is correct? #306

How do we manually verify that the expected value in the unittests is correct? #306

Comments

jlewi commented Aug 23, 2019

jlewi commented Aug 23, 2019

kkasravi commented Aug 23, 2019 • edited Loading

kkasravi commented Aug 23, 2019

jlewi commented Aug 24, 2019

kkasravi commented Aug 25, 2019

jlewi commented Aug 25, 2019

kkasravi commented Aug 25, 2019

jlewi commented Aug 25, 2019

kkasravi commented Aug 26, 2019

jlewi commented Aug 27, 2019

kkasravi commented Aug 27, 2019

jlewi commented Dec 13, 2019

krishnadurai commented Dec 13, 2019

jtfogarty commented Jan 6, 2020

jlewi commented Mar 15, 2020

jlewi commented Mar 16, 2020

krishnadurai commented Mar 16, 2020

jlewi commented Mar 16, 2020

krishnadurai commented Mar 16, 2020 • edited Loading

jlewi commented Mar 25, 2020

kkasravi commented Aug 23, 2019 •

edited

Loading

krishnadurai commented Mar 16, 2020 •

edited

Loading