Add tests for blueprint validate of kanister function with non default versions #1404

viveksinghggits · 2022-04-22T11:51:29Z

Change Overview

This PR adds test for blueprint validation logic for the functions that are not registered with default function version.

Pull request type

Please check the type of change your PR introduces:

Issues

#XXX

Test Plan

💪 Manual
⚡ Unit test
💚 E2E

 » go test 
Failed the 'validation of phase 00 in action backup' check.. ❌
Passed the 'validation of phase 10 in action backup' check.. ✅
Passed the 'validation of phase 11 in action backup' check.. ✅
Failed the 'validation of action backup' check.. ❌
Passed the 'validation of phase 30 in action backup' check.. ✅
Failed the 'validation of phase 40 in action backup' check.. ❌
Failed the 'validation of phase 51 in action backup' check.. ❌
Failed the 'validation of phase 61 in action backup' check.. ❌
Passed the 'validation of phase 71 in action backup' check.. ✅
Passed the 'validation of phase 70 in action backup' check.. ✅
{"FallbackVersion":"v0.0.0","File":"pkg/phase.go","Function":"PrepareData","Line":160,"PreferredVersion":"v0.0.1","cluster_name":"dashboardbff","level":"info","msg":"Falling back to default version of the function","service_name":"dashboardbff","time":"2022-04-22T17:21:18.08268198+05:30"}
Passed the 'validation of phase 00 in action backup' check.. ✅
Passed the 'validation of phase 01 in action backup' check.. ✅
{"FallbackVersion":"v0.0.0","File":"pkg/phase.go","Function":"PrepareData","Line":160,"PreferredVersion":"v0.0.1","cluster_name":"dashboardbff","level":"info","msg":"Falling back to default version of the function","service_name":"dashboardbff","time":"2022-04-22T17:21:18.082894839+05:30"}
Failed the 'validation of phase 10 in action backup' check.. ❌
Passed the 'validation of phase 20 in action backup' check.. ✅
Failed the 'validation of phase 21 in action backup' check.. ❌
{"FallbackVersion":"v0.0.0","File":"pkg/phase.go","Function":"PrepareData","Line":160,"PreferredVersion":"v0.0.1","cluster_name":"dashboardbff","level":"info","msg":"Falling back to default version of the function","service_name":"dashboardbff","time":"2022-04-22T17:21:18.082994938+05:30"}
{"FallbackVersion":"v0.0.0","File":"pkg/phase.go","Function":"PrepareData","Line":160,"PreferredVersion":"v0.0.1","cluster_name":"dashboardbff","level":"info","msg":"Falling back to default version of the function","service_name":"dashboardbff","time":"2022-04-22T17:21:18.083027675+05:30"}
Passed the 'validation of phase 11 in action backup' check.. ✅
Passed the 'validation of phase 11 in action backup' check.. ✅
OK: 2 passed
PASS
ok  	github.com/kanisterio/kanister/pkg/blueprint/validate	0.028s

ihcsim · 2022-04-27T15:48:23Z

Feels like TestValidate() and TestValidateNonDefaultVersion() are almost the same. Can we just refactor TestValidate() to test two versions of nonDefaultVersionFunc, and not bother with PrepareData? Will something like the following work? I feel like we are having a lot of repetition in our test code.

diff --git a/pkg/blueprint/validate/validate_test.go b/pkg/blueprint/validate/validate_test.go
index 6c9ee6f2..b6e4cbc5 100644
--- a/pkg/blueprint/validate/validate_test.go
+++ b/pkg/blueprint/validate/validate_test.go
@@ -223,11 +223,14 @@ func (v *ValidateBlueprint) TestValidate(c *C) {
 		if tc.deferPhase != nil {
 			bp.Actions["backup"].DeferPhase = tc.deferPhase
 		}
-		err := Do(bp, kanister.DefaultVersion)
-		if err != nil {
-			c.Assert(strings.Contains(err.Error(), tc.errContains), Equals, true)
+
+		for _, v := range []string{kanister.DefaultVersion, nonDefaultFuncVersion} {
+			err := Do(bp, v)
+			if err != nil {
+				c.Assert(strings.Contains(err.Error(), tc.errContains), Equals, true)
+			}
+			c.Assert(err, tc.err)
 		}
-		c.Assert(err, tc.err)
 	}
 }
 
@@ -373,4 +376,5 @@ var _ kanister.Func = (*nonDefaultVersionFunc)(nil)
 
 func init() {
 	_ = kanister.RegisterVersion(&nonDefaultVersionFunc{}, nonDefaultFuncVersion)
+	_ = kanister.RegisterVersion(&nonDefaultVersionFunc{}, kanister.DefaultVersion)
 }

pavannd1 · 2022-04-27T22:03:22Z

@ihcsim I think we can optimize this as per your suggestion 👍🏼 Should we follow up later once Vivek is back?

viveksinghggits · 2022-04-28T02:12:37Z

@ihcsim I think we can optimize this as per your suggestion 👍🏼 Should we follow up later once Vivek is back?

I will push the change later today.

viveksinghggits · 2022-05-24T07:10:05Z

Hi @ihcsim,
I tried to make the change and I think it's ok to have the tests as they are because of below reasons

Testing blueprint against both the versions is not a good idea, in general, I think. Because when we actually create the blueprint we just validate the blueprint instead of validating it against different function versions.
A blueprint that has functions that are registered with non default function version would not pass the validation if we validate it against default function version. In that case we will have to maintain two error scenarios for the test cases, one for validation with default version and other for validation against non default version.

On another context, by duplicate you mean the test cases are duplicate of the code that validates them.

viveksinghggits · 2022-05-25T10:13:01Z

cc @Shrekster

ihcsim · 2022-05-26T21:12:13Z

Testing blueprint against both the versions is not a good idea, in general, I think.

Not sure if I followed. We added a test case to validate a blueprint containing one function with default version and one function with non-default version. Why can't we test with one function of different versions? I am not suggesting we validate differences between two versions of the same function.

Are you saying this is not a valid usage pattern 👇 ?

 func init() {
 	_ = kanister.RegisterVersion(&myTestFunc{}, nonDefaultFuncVersion)
	_ = kanister.RegisterVersion(&myTestFunc{}, kanister.DefaultVersion)
 }

A blueprint that has functions that are registered with non default function version would not pass the validation if we validate it against default function version.

Can you link to the code that does this?

viveksinghggits · 2022-05-27T04:49:54Z

In future we might have some use case where a function is registered in both the versions but for now I don't think we have examples where a kanister function is registered with both the versions.

 func init() {
 	_ = kanister.RegisterVersion(&myTestFunc{}, nonDefaultFuncVersion)
	_ = kanister.RegisterVersion(&myTestFunc{}, kanister.DefaultVersion)
 }

so its not invalid but we don't have the examples as of now. And even if we were doing that, the main thing is we don't validate a blueprint against different function versions. We just pass one version while validating a blueprint and if a function is not available in version we fallback to default function version.
2. https://github.com/kanisterio/kanister/blob/master/pkg/phase.go#L123
so when a blueprint has a function that is registered with nondefault function version and we validate that blueprint against default function version, the map that maintains the function and their versions will not return anything and since we are checking default function version we will not fall back to anything else so that function will not be found.

viveksinghggits · 2022-06-03T17:24:46Z

Talked to @ihcsim about this. Kueueing this PR now.

ihcsim

@viveksinghggits Thanks for the discussion. To limit the scope of this PR, it makes sense to separate the original validation test from the new version-related validation.

I think at one point, we might have to re-visit the support for multi-versions Kanister Function scenario.

viveksinghggits · 2022-06-03T17:31:47Z

I think at one point, we might have to re-visit the support for multi-versions Kanister Function scenario.

I agree.

Add tests for blueprint validate of non default kanister versions

55f70a7

viveksinghggits requested review from pavannd1, PrasadG193 and ihcsim April 22, 2022 11:52

Fix lint issue

761857f

pavannd1 approved these changes Apr 26, 2022

View reviewed changes

viveksinghggits added the kueue label Apr 27, 2022

Merge branch 'master' into non-def-val

ce83a91

viveksinghggits added the hold off merging label Apr 28, 2022

Fix minor typo

38663fe

viveksinghggits removed the hold off merging label Jun 3, 2022

Merge branch 'master' into non-def-val

ade4306

ihcsim approved these changes Jun 3, 2022

View reviewed changes

mergify bot merged commit c5acaac into master Jun 3, 2022

mergify bot deleted the non-def-val branch June 3, 2022 17:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for blueprint validate of kanister function with non default versions #1404

Add tests for blueprint validate of kanister function with non default versions #1404

viveksinghggits commented Apr 22, 2022

ihcsim commented Apr 27, 2022 •

edited

Loading

pavannd1 commented Apr 27, 2022

viveksinghggits commented Apr 28, 2022

viveksinghggits commented May 24, 2022

viveksinghggits commented May 25, 2022

ihcsim commented May 26, 2022 •

edited

Loading

viveksinghggits commented May 27, 2022

viveksinghggits commented Jun 3, 2022

ihcsim left a comment

viveksinghggits commented Jun 3, 2022

Add tests for blueprint validate of kanister function with non default versions #1404

Add tests for blueprint validate of kanister function with non default versions #1404

Conversation

viveksinghggits commented Apr 22, 2022

Change Overview

Pull request type

Issues

Test Plan

ihcsim commented Apr 27, 2022 • edited Loading

pavannd1 commented Apr 27, 2022

viveksinghggits commented Apr 28, 2022

viveksinghggits commented May 24, 2022

viveksinghggits commented May 25, 2022

ihcsim commented May 26, 2022 • edited Loading

viveksinghggits commented May 27, 2022

viveksinghggits commented Jun 3, 2022

ihcsim left a comment

Choose a reason for hiding this comment

viveksinghggits commented Jun 3, 2022

ihcsim commented Apr 27, 2022 •

edited

Loading

ihcsim commented May 26, 2022 •

edited

Loading