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

Fix HelmChart values tests #332

Merged
merged 1 commit into from
Apr 9, 2021
Merged

Fix HelmChart values tests #332

merged 1 commit into from
Apr 9, 2021

Conversation

arbourd
Copy link
Contributor

@arbourd arbourd commented Apr 4, 2021

Changes two tests that load the HelmChart from the updated resource and verifies that testOverride (the value overrode in the test fixtures) changes from false to true.

  1. Paths were corrected from ./charts/... to ./testdata/charts/... (thank you @relu)
  2. Updates the updated HelmChart, not chart which is from some other test further up the spec (thank you @nniikkoollaaii)
  3. Both tests need to wait for Helm chart reconciliation. I accomplished that with got.Status.Artifact.Checksum != updated.Status.Artifact.Checksum
  4. The first test needed to be moved outside of "Creates artifacts with .tgz file" because this tarball is never extracted (thank you @nniikkoollaaii). It was moved one step above to "Creates artifacts for" which tests the revision changes of testdata/chart/helmchart
  5. Adds a test to ensure the filesize is greater than zero.

@arbourd arbourd changed the title Test values overrides Test override values Apr 4, 2021
@nniikkoollaaii
Copy link

These tests fail for multiple reasons.

First: Without changing Line 700 from chart.Spec.ValuesFile = "./charts/helmchart/override.yaml" to updated.Spec.ValuesFile = "./charts/helmchart/override.yaml" nothing happens. The function reconcileFromTarballArtifact() isn't called.

Second: After changing to updated the method reconcileFromTarballArtifact() returns at line 518 because the valuesFile was not found in tmpDir. This isn't because of a invalid path. This is because there is no directory "helmchart" in the tmpDir containing the unpacked tarball.

@arbourd is sure this works in their test environment. Could this be a problem with the unit test setup? I'm unable to find the reason :/

@relu
Copy link
Member

relu commented Apr 6, 2021

Please see my comment: #305 (comment)

@hiddeco hiddeco added the area/ci CI related issues and pull requests label Apr 7, 2021
Comment on lines 621 to 628
Eventually(func() bool {
_ = k8sClient.Get(context.Background(), key, got)
return got.Status.Artifact != nil && storage.ArtifactExist(*got.Status.Artifact)
}, timeout, interval).Should(BeTrue())
f, err := os.Stat(storage.LocalPath(*got.Status.Artifact))
Expect(err).NotTo(HaveOccurred())
Expect(f.Size()).NotTo(BeZero())
Expect(got.Status.Artifact.Revision).To(Equal(updated.Status.Artifact.Revision))
Copy link
Member

Choose a reason for hiding this comment

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

I do not think that this waits for the update to be processed, but instead just confirms the previous state. The best way to confirm the change has actually been processed before confirming state is likely to confirm the Status.ObservedGeneration of got has changed compared to updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I was thinking that might be the correct answer for the valid tests as well, instead of Checksum.

Expect(f.Size()).To(BeNumerically(">", 0))
helmChart, err := loader.Load(storage.LocalPath(*got.Status.Artifact))
Expect(err).NotTo(HaveOccurred())
Expect(helmChart.Values["testOverride"]).To(BeTrue())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

testOverride remains true because the invalid chart change does not break last working state.

@arbourd arbourd changed the title Test override values Fix HelmChart values tests Apr 8, 2021
Copy link
Member

@relu relu left a comment

Choose a reason for hiding this comment

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

LGTM ✔️

Thank you @arbourd!

Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

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

LGTM as well, thanks a lot! 🌻

Can you please rebase?

Adds a test that loads the helmChart from the updated resource and
verifies that `testOverride` (the value overrode in the test fixtures)
changes from `false` to `true`.

Signed-off-by: Dylan Arbour <arbourd@users.noreply.github.com>
@arbourd
Copy link
Contributor Author

arbourd commented Apr 8, 2021

Done @hiddeco. Thank you. 😌

@hiddeco hiddeco merged commit 47492c4 into fluxcd:main Apr 9, 2021
@arbourd arbourd deleted the add-values-test branch April 9, 2021 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ci CI related issues and pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants