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 unit tests for kustomize #1828

Merged
merged 1 commit into from
Mar 19, 2019

Conversation

dgageot
Copy link
Contributor

@dgageot dgageot commented Mar 19, 2019

Signed-off-by: David Gageot [email protected]

Signed-off-by: David Gageot <[email protected]>
@codecov-io
Copy link

codecov-io commented Mar 19, 2019

Codecov Report

Merging #1828 into master will increase coverage by 0.74%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1828      +/-   ##
==========================================
+ Coverage   45.42%   46.17%   +0.74%     
==========================================
  Files         143      143              
  Lines        6683     6685       +2     
==========================================
+ Hits         3036     3087      +51     
+ Misses       3341     3287      -54     
- Partials      306      311       +5
Impacted Files Coverage Δ
pkg/skaffold/deploy/kustomize.go 62.4% <100%> (+40.44%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f6c7f56...88c84a8. Read the comment docs.

Copy link
Contributor

@nkubala nkubala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one small nit but otherwise 👍

command: testutil.NewFakeCmd(t).
WithRunOut("kubectl version --client -ojson", kubectlVersion).
WithRunOut("kustomize build "+tmpDir.Root(), deploymentWebYAML).
WithRun("kubectl --context kubecontext --namespace testNamespace apply --force -f -"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a huge deal, but can you use the variables testKubeContext and testNamespace here instead of the strings?

WithRun("kubectl --context " + testKubeContext + " -- namespace " + testNamespace + " apply --force -f -")

or

WithRun(fmt.Sprintf("kubectl --context %s --namespace %s apply --force -f -", testKubeContext, testNamespace))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know about that. Those strings are basically assertions and I'd rather hard code the expected result than compute it. I know it's debatable...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah ok I misunderstood, I thought you were actually generating the command to run here. it does feel weird to me that this test will break if we rename a variable somewhere...but I see where you're coming from. I don't feel strongly enough about it to push for you to change it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@dgageot dgageot merged commit c458ab4 into GoogleContainerTools:master Mar 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants