Add simple external test covering cache verification #118

josvazg · 2021-12-14T19:12:13Z

Signed-off-by: Jose Luis Vazquez Gonzalez [email protected]

This is the simplest way I can think of we ensure that, at the very least, if the cache stops working as expected we will notice in our testing. Deeper testing would be a lot more costly in time to what I can invest right now.

josvazg · 2021-12-15T07:08:52Z

I should add a small sub test to ensure a second usage of the layer uses the cached file this time.

petewall · 2021-12-15T23:13:38Z

test/external/external_test.go

+		define.When(`^removing relok8s-save-cache$`, func() {
+			err := os.RemoveAll(saveCacheFolder())
+			Expect(err).ToNot(HaveOccurred())
+		})


If this is just a cleanup step, you can add a second func() that runs after all of the regular steps.

http://www.bunniesandbeatings.com/goerkin/#cleanup-steps

Actually this is not really a clean up. I will remove this to clear relok8s-save-cache because this is a pre-step before testing the cache. This ensures the first run is not using cached content, yet the cached content gets populated:

steps.When("clear relok8s-save-cache") steps.When(fmt.Sprintf("running relok8s chart move -y ../fixtures/testchart --image-patterns ../fixtures/testchart.images.yaml --to-intermediate-bundle %s/testchart-intermediate.tar", tmpDir)) ... steps.Then("relok8s-save-cache contains expected layer")

Still, I did not know the cleanup trick for goerkin, and it might be useful elsewhere.

josvazg · 2021-12-16T07:13:12Z

Abandoned in favor of #119

josvazg · 2022-01-10T08:22:17Z

#119 got rejected so we resume this again to have test coverage of the caching support.

migmartri

Please don't update the headers of files that have not been updated.

did you get a error when running the linter locally or in the CI? note that I changed the github action to only take into account what has changed, locally it's by default like that.

Signed-off-by: Jose Luis Vazquez Gonzalez <[email protected]>

josvazg · 2022-01-10T15:29:35Z

Please don't update the headers of files that have not been updated.

did you get a error when running the linter locally or in the CI? note that I changed the github action to only take into account what has changed, locally it's by default like that.

Done, I have removed the header renames also from this PR. I updated the header only on the file touched here.

migmartri

Thanks!

migmartri · 2022-01-11T12:25:24Z

test/external/external_test.go

@@ -19,6 +19,15 @@ import (
 	. "github.com/onsi/gomega/gbytes"
 )

+func saveCacheFolder() string {
+	return filepath.Join(os.TempDir(), "relok8s-save-cache")


This is actually deleting the cache that's being used by relok8s itself right? I don't like that running a test has the side-effect of deleting the actual tooling cache.

Could we instead run these tests pointing to an unique, per-test cache path? This might require adding support for our tool via ENV variable or flag to point to the cache directory.

Do not want to add more code to be able to test, we already abandoned a change for that.

Maybe I can remove the specific layer instead for the test.

nah, don't worry about it, we can live with that side-effect I guess

migmartri · 2022-01-11T12:26:30Z

test/external/external_test.go

+		modtime := info.ModTime()
+		steps.When(fmt.Sprintf("running relok8s chart move -y ../fixtures/testchart --image-patterns ../fixtures/testchart.images.yaml --to-intermediate-bundle %s/testchart-intermediate-2.tar", tmpDir))
+		steps.And("the move is computed")
+		steps.Then("the command says it will archive the chart")


Is there much value on again checking the output of the command? Could we just check on the second run that it does not expose any error and that the layer is there?

migmartri · 2022-01-11T12:28:37Z

test/external/external_test.go

+		info, err = os.Stat(expectedLayerFile())
+		Expect(err).ToNot(HaveOccurred())
+		Expect(info.ModTime()).To(Equal(modtime))
+		Expect(secondSaveDuration).To(BeNumerically("<", firstSaveDuration))


This test case can be brittle, what if extracting the file from local takes more than the network speed? It should not be the case in general but you are adding a dependency here to the underlying system.

I guess we could remove this, checking the file mod time did not change means the layer was not overwritten and we can kind of assume it was reused by the cache.

Speaking of brittle tests, while I have not seen this one fail yet, I keep on seeing that unit test fail often:

--- FAIL: TestGroupChangesByChart (0.00s) panic: runtime error: index out of range [0] with length 0 [recovered] panic: runtime error: index out of range [0] with length 0

And that is on top of the harbor issue that fails some external tests sometimes.

timing checks removed

--- FAIL: TestGroupChangesByChart (0.00s)
panic: runtime error: index out of range [0] with length 0 [recovered]
panic: runtime error: index out of range [0] with length 0

yep, i have it on my plate to take a look at it. I looked into it the other day but didn't manage to reproduce it

It is very weird indeed, it happened a few times to me for a little while, then did not happen again for some time. It looks like some kind of race condition somewhere.

Signed-off-by: Jose Luis Vazquez Gonzalez <[email protected]>

migmartri

LGTM!

migmartri · 2022-01-11T15:53:10Z

test/external/external_test.go

-		define.Then(`^remove the archive folder at testchart-intermediate.tar$`, func() {
-			err := os.Remove("testchart-intermediate.tar")
+		define.When(`^clear relok8s-save-cache expected layer$`, func() {
+			err := os.RemoveAll(expectedLayerFile())


nice, thanks!

josvazg requested review from migmartri and petewall December 14, 2021 19:12

josvazg self-assigned this Dec 14, 2021

josvazg mentioned this pull request Dec 15, 2021

Add caching stats & tests #119

Closed

petewall reviewed Dec 15, 2021

View reviewed changes

josvazg closed this Dec 16, 2021

josvazg reopened this Jan 10, 2022

josvazg force-pushed the test-save-cache branch from aac8f05 to 55ee8d8 Compare January 10, 2022 08:47

migmartri suggested changes Jan 10, 2022

View reviewed changes

josvazg mentioned this pull request Jan 10, 2022

Update headers - it's 2022 now #124

Closed

Jose Luis Vazquez Gonzalez added 3 commits January 10, 2022 16:25

Add simple external test covering cache verification

ecfabd4

Signed-off-by: Jose Luis Vazquez Gonzalez <[email protected]>

Add cached usage test

30e7a67

Signed-off-by: Jose Luis Vazquez Gonzalez <[email protected]>

Reword cache pre-clearing action

504785c

Signed-off-by: Jose Luis Vazquez Gonzalez <[email protected]>

josvazg force-pushed the test-save-cache branch from 55ee8d8 to 504785c Compare January 10, 2022 15:27

josvazg requested review from migmartri and petewall January 10, 2022 15:52

migmartri approved these changes Jan 11, 2022

View reviewed changes

Jose Luis Vazquez Gonzalez added 3 commits January 11, 2022 16:43

Remove extra output checks on second cache test run

ae752f9

Signed-off-by: Jose Luis Vazquez Gonzalez <[email protected]>

Remove caching test timing checks

0c12052

Signed-off-by: Jose Luis Vazquez Gonzalez <[email protected]>

Remove only the expected layer for the caching test

a591443

Signed-off-by: Jose Luis Vazquez Gonzalez <[email protected]>

migmartri approved these changes Jan 11, 2022

View reviewed changes

josvazg merged commit f519eb3 into main Jan 12, 2022

josvazg mentioned this pull request Jan 12, 2022

Airgap, environment scenario #15

Closed

migmartri deleted the test-save-cache branch January 12, 2022 09:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add simple external test covering cache verification #118

Add simple external test covering cache verification #118

josvazg commented Dec 14, 2021

josvazg commented Dec 15, 2021

petewall Dec 15, 2021

josvazg Jan 10, 2022

josvazg Jan 10, 2022

josvazg commented Dec 16, 2021

josvazg commented Jan 10, 2022

migmartri left a comment

josvazg commented Jan 10, 2022 •

edited

Loading

migmartri left a comment

migmartri Jan 11, 2022

josvazg Jan 11, 2022 •

edited

Loading

migmartri Jan 11, 2022 •

edited

Loading

josvazg Jan 11, 2022

migmartri Jan 11, 2022

josvazg Jan 11, 2022

josvazg Jan 11, 2022

migmartri Jan 11, 2022

josvazg Jan 11, 2022

josvazg Jan 11, 2022

migmartri Jan 11, 2022

josvazg Jan 12, 2022

migmartri left a comment

migmartri Jan 11, 2022

Add simple external test covering cache verification #118

Add simple external test covering cache verification #118

Conversation

josvazg commented Dec 14, 2021

josvazg commented Dec 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josvazg commented Dec 16, 2021

josvazg commented Jan 10, 2022

migmartri left a comment

Choose a reason for hiding this comment

josvazg commented Jan 10, 2022 • edited Loading

migmartri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josvazg Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

migmartri Jan 11, 2022 • 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

migmartri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josvazg commented Jan 10, 2022 •

edited

Loading

josvazg Jan 11, 2022 •

edited

Loading

migmartri Jan 11, 2022 •

edited

Loading