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

[jlewi]

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

[jlewi]

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]

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]

@jlewi I've opened #308 PTAL

[jlewi]

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]

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

[jlewi]

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]

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]

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]

ok, I'll implement in #308

[jlewi]

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]

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