From 20b558656d7d369e8317a3ce1dd20709ddc53638 Mon Sep 17 00:00:00 2001 From: "James O. D. Hunt" Date: Thu, 9 May 2019 14:56:38 +0100 Subject: [PATCH 1/4] katatestutils: Reset TestConstraints fields on NotValid() call `TestConstraint.NotValid()` is really designed to be called once per test. However, there is no reason it should not be possible to call multiple times. But to allow for that secenario, any settings from a previous `NotValid()` call need to be cleared. Signed-off-by: James O. D. Hunt --- pkg/katatestutils/constraints_api.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pkg/katatestutils/constraints_api.go b/pkg/katatestutils/constraints_api.go index 19c4005fc7..d135a06f4e 100644 --- a/pkg/katatestutils/constraints_api.go +++ b/pkg/katatestutils/constraints_api.go @@ -112,6 +112,11 @@ func (tc *TestConstraint) NotValid(constraints ...Constraint) bool { panic("need atleast one constraint") } + // Reset in case of a previous call + tc.Passed = nil + tc.Failed = nil + tc.Issue = "" + for _, c := range constraints { invalid := tc.constraintInvalid(c) if invalid { From 1bec735cb61e49b0f848578f2e8afeb053a737e6 Mon Sep 17 00:00:00 2001 From: "James O. D. Hunt" Date: Thu, 9 May 2019 15:00:21 +0100 Subject: [PATCH 2/4] katatestutils: Fix NeedDistroVersionEquals comment Previously, the comment on `NeedDistroVersionEquals()` erroneously referred to `NeedDistroVersionLT()`. Signed-off-by: James O. D. Hunt --- pkg/katatestutils/constraints_api.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/katatestutils/constraints_api.go b/pkg/katatestutils/constraints_api.go index d135a06f4e..d97266700f 100644 --- a/pkg/katatestutils/constraints_api.go +++ b/pkg/katatestutils/constraints_api.go @@ -182,8 +182,8 @@ func NeedDistroVersionWithOp(version string, op Operator) Constraint { } } -// NeedDistroVersionLT will skip the test unless the distro version is older -// than the specified version. +// NeedDistroVersionEquals will skip the test unless the distro version is the +// same as the specified version. // // Note: distro versions vary in format. func NeedDistroVersionEquals(version string) Constraint { From 2bc03f23e01375aeda034e2a853c7321c94b7a7a Mon Sep 17 00:00:00 2001 From: "James O. D. Hunt" Date: Thu, 9 May 2019 15:31:49 +0100 Subject: [PATCH 3/4] katatestutils: Fix version check bug Fix version comparison bug whereby the specified operator was being applied to the wrong version number. The version handling for distro and kernel versions was incorrect. This was not clear as the internal logic was rather difficult to understand. Renaming `constraintInvalid()` to `constraintValid()` and updating `NotValid()` correspondingly makes the logic clearer and exposed the bug, allowing it to be fixed. Added two new tests to ensure correct behaviour: - `TestConstraintNotValidKernelVersion()` - `TestConstraintNotValidDistroVersion()` Fixes #1653. Signed-off-by: James O. D. Hunt --- pkg/katatestutils/constraints.go | 22 +-- pkg/katatestutils/constraints_api.go | 4 +- pkg/katatestutils/constraints_test.go | 220 ++++++++++++++++++++++---- 3 files changed, 205 insertions(+), 41 deletions(-) diff --git a/pkg/katatestutils/constraints.go b/pkg/katatestutils/constraints.go index 68d92b2d0e..5ac50de1d0 100644 --- a/pkg/katatestutils/constraints.go +++ b/pkg/katatestutils/constraints.go @@ -184,13 +184,13 @@ func (tc *TestConstraint) handleDistroName(name string, op Operator) (result Res // handleDistroVersion checks that the current distro version is compatible with // the constraint specified by the arguments. func (tc *TestConstraint) handleDistroVersion(version string, op Operator) (result Result, err error) { - return handleVersionType("distro", version, op, tc.DistroVersion) + return handleVersionType("distro", tc.DistroVersion, op, version) } // handleKernelVersion checks that the current kernel version is compatible with // the constraint specified by the arguments. func (tc *TestConstraint) handleKernelVersion(version string, op Operator) (result Result, err error) { - return handleVersionType("kernel", version, op, tc.KernelVersion) + return handleVersionType("kernel", tc.KernelVersion, op, version) } // handleVersionType checks that the current and new versions are compatible with @@ -250,7 +250,7 @@ func handleVersionType(versionName, newVersion string, op Operator, currentVersi } descr := fmt.Sprintf("need %s version %s %q, got version %q", - versionName, op, newVersion, currentVersion) + versionName, op, currentVersion, newVersion) result = Result{ Description: descr, @@ -432,9 +432,9 @@ func (tc *TestConstraint) handleResults(result Result, err error) { } } -// constraintInvalid handles the specified constraint, returning true if the -// constraint fails, else false. -func (tc *TestConstraint) constraintInvalid(fn Constraint) bool { +// constraintValid handles the specified constraint, returning true if the +// constraint is valid, else false. +func (tc *TestConstraint) constraintValid(fn Constraint) bool { c := Constraints{} // Call the constraint function that sets the Constraints values @@ -449,7 +449,7 @@ func (tc *TestConstraint) constraintInvalid(fn Constraint) bool { result, err := tc.handleUID(c.UID, c.Operator) tc.handleResults(result, err) if !result.Success { - return true + return false } } @@ -457,7 +457,7 @@ func (tc *TestConstraint) constraintInvalid(fn Constraint) bool { result, err := tc.handleDistroName(c.DistroName, c.Operator) tc.handleResults(result, err) if !result.Success { - return true + return false } } @@ -465,7 +465,7 @@ func (tc *TestConstraint) constraintInvalid(fn Constraint) bool { result, err := tc.handleDistroVersion(c.DistroVersion, c.Operator) tc.handleResults(result, err) if !result.Success { - return true + return false } } @@ -474,11 +474,11 @@ func (tc *TestConstraint) constraintInvalid(fn Constraint) bool { result, err := tc.handleKernelVersion(c.KernelVersion, c.Operator) tc.handleResults(result, err) if !result.Success { - return true + return false } } // Constraint is valid - return false + return true } diff --git a/pkg/katatestutils/constraints_api.go b/pkg/katatestutils/constraints_api.go index d97266700f..2c37d7d600 100644 --- a/pkg/katatestutils/constraints_api.go +++ b/pkg/katatestutils/constraints_api.go @@ -118,8 +118,8 @@ func (tc *TestConstraint) NotValid(constraints ...Constraint) bool { tc.Issue = "" for _, c := range constraints { - invalid := tc.constraintInvalid(c) - if invalid { + valid := tc.constraintValid(c) + if !valid { return true } } diff --git a/pkg/katatestutils/constraints_test.go b/pkg/katatestutils/constraints_test.go index d556ba3f01..c13fcb3a15 100644 --- a/pkg/katatestutils/constraints_test.go +++ b/pkg/katatestutils/constraints_test.go @@ -196,19 +196,34 @@ func semverBumpVersion(ver semver.Version, bumpMajor, bumpMinor, bumpPatch bool) return ver.String(), nil } -// incrementVersion increments the specified version and returns the -// string representation. -func incrementVersion(version string) (string, error) { +// changeVersion modifies the specified version and returns the +// string representation. If decrement is true the returned version is smaller +// than the specified version, else it is larger. +func changeVersion(version string, decrement bool) (string, error) { + operand := int64(1) + + if decrement { + operand = -1 + } + // Is it an integer? intResult, err := strconv.ParseUint(version, 10, 0) if err == nil { - return fmt.Sprintf("%d", intResult+1), nil + if intResult == 0 && decrement { + return "", fmt.Errorf("cannot decrement integer version with value zero") + } + + return fmt.Sprintf("%d", uint64(int64(intResult)+operand)), nil } // Is it a float? floatResult, err := strconv.ParseFloat(version, 32) if err == nil { - return fmt.Sprintf("%f", floatResult+1), nil + if int(floatResult) == 0 && decrement { + return "", fmt.Errorf("cannot decrement integer part of floating point version with value zero: %v", version) + } + + return fmt.Sprintf("%f", floatResult+float64(operand)), nil } // Not an int nor a float, so it must be a semantic version @@ -218,14 +233,36 @@ func incrementVersion(version string) (string, error) { return "", err } - err = ver.IncrementMajor() - if err != nil { - return "", err + if decrement { + // the semver package only provides increment operations, so + // handle decrement ourselves. + major := ver.Major + + if major == 0 { + return "", fmt.Errorf("cannot decrement semver with zero major version: %+v", version) + } + + major-- + + ver.Major = major + } else { + err = ver.IncrementMajor() + if err != nil { + return "", err + } } return ver.String(), nil } +func incrementVersion(version string) (string, error) { + return changeVersion(version, false) +} + +func decrementVersion(version string) (string, error) { + return changeVersion(version, true) +} + // testGetDistro is an alternative implementation of getDistroDetails() used // for testing. func testGetDistro() (name, version string, err error) { @@ -550,16 +587,16 @@ func TestConstraintHandleDistroVersion(t *testing.T) { {higherVersion, eqOperator, Result{Success: false}, false}, {distroVersion, gtOperator, Result{Success: false}, false}, - {higherVersion, gtOperator, Result{Success: true}, false}, + {higherVersion, gtOperator, Result{Success: false}, false}, {distroVersion, geOperator, Result{Success: true}, false}, - {higherVersion, geOperator, Result{Success: true}, false}, + {higherVersion, geOperator, Result{Success: false}, false}, {distroVersion, ltOperator, Result{Success: false}, false}, - {higherVersion, ltOperator, Result{Success: false}, false}, + {higherVersion, ltOperator, Result{Success: true}, false}, {distroVersion, leOperator, Result{Success: true}, false}, - {higherVersion, leOperator, Result{Success: false}, false}, + {higherVersion, leOperator, Result{Success: true}, false}, {distroVersion, neOperator, Result{Success: false}, false}, {higherVersion, neOperator, Result{Success: true}, false}, @@ -773,24 +810,24 @@ func TestConstraintHandleKernelVersion(t *testing.T) { {kernelVersion, neOperator, Result{Success: false}, false}, {newerMajor, eqOperator, Result{Success: false}, false}, - {newerMajor, geOperator, Result{Success: true}, false}, - {newerMajor, gtOperator, Result{Success: true}, false}, - {newerMajor, ltOperator, Result{Success: false}, false}, - {newerMajor, leOperator, Result{Success: false}, false}, + {newerMajor, geOperator, Result{Success: false}, false}, + {newerMajor, gtOperator, Result{Success: false}, false}, + {newerMajor, ltOperator, Result{Success: true}, false}, + {newerMajor, leOperator, Result{Success: true}, false}, {newerMajor, neOperator, Result{Success: true}, false}, {newerMinor, eqOperator, Result{Success: false}, false}, - {newerMinor, geOperator, Result{Success: true}, false}, - {newerMinor, gtOperator, Result{Success: true}, false}, - {newerMinor, ltOperator, Result{Success: false}, false}, - {newerMinor, leOperator, Result{Success: false}, false}, + {newerMinor, geOperator, Result{Success: false}, false}, + {newerMinor, gtOperator, Result{Success: false}, false}, + {newerMinor, ltOperator, Result{Success: true}, false}, + {newerMinor, leOperator, Result{Success: true}, false}, {newerMinor, neOperator, Result{Success: true}, false}, {newerPatch, eqOperator, Result{Success: false}, false}, - {newerPatch, geOperator, Result{Success: true}, false}, - {newerPatch, gtOperator, Result{Success: true}, false}, - {newerPatch, ltOperator, Result{Success: false}, false}, - {newerPatch, leOperator, Result{Success: false}, false}, + {newerPatch, geOperator, Result{Success: false}, false}, + {newerPatch, gtOperator, Result{Success: false}, false}, + {newerPatch, ltOperator, Result{Success: true}, false}, + {newerPatch, leOperator, Result{Success: true}, false}, {newerPatch, neOperator, Result{Success: true}, false}, } @@ -1145,7 +1182,134 @@ func TestConstraintNotValid(t *testing.T) { } -func TestConstraintConstraintInvalid(t *testing.T) { +func TestConstraintNotValidKernelVersion(t *testing.T) { + assert := assert.New(t) + + assert.NotNil(kernelVersion) + + // Generate new kernel versions for testing purposes based on the + // current kernel version. + higherVersion, err := incrementVersion(kernelVersion) + assert.NoError(err) + assert.NotEqual(kernelVersion, higherVersion) + + lowerVersion, err := decrementVersion(kernelVersion) + assert.NoError(err) + assert.NotEqual(kernelVersion, lowerVersion) + + // Antique kernel version numbers. + // + // Note: Not all are actually real kernel releases - we're just trying + // to do a thorough test. + lowKernelVersions := []string{ + "0.0.0", + "0.0.1", + "1.0.0", + "1.0.6-1.1.0", + "2.0.0", + "2.6.0", + lowerVersion, + } + + // Host kernel is expected to be newer than all the low kernel versions + for _, debug := range []bool{true, false} { + tc := NewTestConstraint(debug) + + for _, ver := range lowKernelVersions { + result := tc.NotValid(NeedKernelVersionEquals(ver)) + assert.True(result) + + result = tc.NotValid(NeedKernelVersionLE(ver)) + assert.True(result) + + result = tc.NotValid(NeedKernelVersionLT(ver)) + assert.True(result) + + result = tc.NotValid(NeedKernelVersionGT(ver)) + assert.False(result) + + result = tc.NotValid(NeedKernelVersionGE(ver)) + assert.False(result) + + result = tc.NotValid(NeedKernelVersionNotEquals(ver)) + assert.False(result) + } + } + + // Ridiculously high kernel version numbers. The host kernel is + // expected to never reach these values. + highKernelVersions := []string{ + higherVersion, + "999.0.0", + "999.0.999", + "999.999.999", + "1024.0.0", + } + + for _, debug := range []bool{true, false} { + tc := NewTestConstraint(debug) + + for _, ver := range highKernelVersions { + result := tc.NotValid(NeedKernelVersionEquals(ver)) + assert.True(result) + + result = tc.NotValid(NeedKernelVersionGE(ver)) + assert.True(result) + + result = tc.NotValid(NeedKernelVersionGT(ver)) + assert.True(result) + + result = tc.NotValid(NeedKernelVersionLE(ver)) + assert.False(result) + + result = tc.NotValid(NeedKernelVersionLT(ver)) + assert.False(result) + + result = tc.NotValid(NeedKernelVersionNotEquals(ver)) + assert.False(result) + } + } +} + +func TestConstraintNotValidDistroVersion(t *testing.T) { + assert := assert.New(t) + + assert.NotNil(distroVersion) + + // Generate new distro versions for testing purposes based on the + // current kernel version. + higherVersion, err := incrementVersion(distroVersion) + assert.NoError(err) + assert.NotEqual(distroVersion, higherVersion) + + lowerVersion, err := decrementVersion(distroVersion) + assert.NoError(err) + assert.NotEqual(distroVersion, lowerVersion) + + for _, debug := range []bool{true, false} { + tc := NewTestConstraint(debug) + + result := tc.NotValid(NeedDistroVersionEquals(higherVersion)) + assert.True(result) + + result = tc.NotValid(NeedDistroVersionEquals(distroVersion)) + assert.False(result) + + result = tc.NotValid(NeedDistroVersionLT(higherVersion)) + assert.False(result) + + result = tc.NotValid(NeedDistroVersionLT(distroVersion)) + assert.True(result) + + result = tc.NotValid(NeedDistroVersionGT(higherVersion)) + assert.True(result) + + result = tc.NotValid(NeedDistroVersionGT(distroVersion)) + assert.True(result) + } +} + +func TestConstraintConstraintValid(t *testing.T) { assert := assert.New(t) type testData struct { @@ -1312,7 +1476,7 @@ func TestConstraintConstraintInvalid(t *testing.T) { for i, d := range data { tc := NewTestConstraint(debug) - result := tc.constraintInvalid(d.fn) + result := tc.constraintValid(d.fn) msg := fmt.Sprintf("test[%d]: %+v, result: %v", i, d, result) @@ -1321,13 +1485,13 @@ func TestConstraintConstraintInvalid(t *testing.T) { } if d.valid { - assert.False(result, msg) + assert.True(result, msg) if len(d.expected.Passed) != 0 { assert.Equal(d.expected.Passed[0].Success, tc.Passed[0].Success, msg) } } else { - assert.True(result, msg) + assert.False(result, msg) if len(d.expected.Failed) != 0 { assert.Equal(d.expected.Failed[0].Success, tc.Failed[0].Success, msg) From 0c207c16ef14ed420e0c98f9943306138f8cf2a0 Mon Sep 17 00:00:00 2001 From: "James O. D. Hunt" Date: Thu, 9 May 2019 15:37:57 +0100 Subject: [PATCH 4/4] katatestutils: Add missing distro version constraints Added the following distro version constraints for parity with the kernel version constraints: - `NeedDistroVersionGE()` - `NeedDistroVersionLE()` - `NeedDistroVersionNotEquals()` Signed-off-by: James O. D. Hunt --- pkg/katatestutils/constraints_api.go | 24 ++++++++++++++++++++++++ pkg/katatestutils/constraints_test.go | 18 ++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/pkg/katatestutils/constraints_api.go b/pkg/katatestutils/constraints_api.go index 2c37d7d600..ce4af56f2e 100644 --- a/pkg/katatestutils/constraints_api.go +++ b/pkg/katatestutils/constraints_api.go @@ -190,6 +190,22 @@ func NeedDistroVersionEquals(version string) Constraint { return NeedDistroVersionWithOp(version, eqOperator) } +// NeedDistroVersionNotEquals will skip the test unless the distro version is +// different to the specified version. +// +// Note: distro versions vary in format. +func NeedDistroVersionNotEquals(version string) Constraint { + return NeedDistroVersionWithOp(version, neOperator) +} + +// NeedDistroVersionLE will skip the test unless the distro version is older +// than or the same as the specified version. +// +// Note: distro versions vary in format. +func NeedDistroVersionLE(version string) Constraint { + return NeedDistroVersionWithOp(version, leOperator) +} + // NeedDistroVersionLT will skip the test unless the distro version is older // than the specified version. // @@ -198,6 +214,14 @@ func NeedDistroVersionLT(version string) Constraint { return NeedDistroVersionWithOp(version, ltOperator) } +// NeedDistroVersionGE will skip the test unless the distro version is newer +// than or the same as the specified version. +// +// Note: distro versions vary in format. +func NeedDistroVersionGE(version string) Constraint { + return NeedDistroVersionWithOp(version, geOperator) +} + // NeedDistroVersionGT will skip the test unless the distro version is newer // than the specified version. // diff --git a/pkg/katatestutils/constraints_test.go b/pkg/katatestutils/constraints_test.go index c13fcb3a15..16bc63cf4b 100644 --- a/pkg/katatestutils/constraints_test.go +++ b/pkg/katatestutils/constraints_test.go @@ -1295,17 +1295,35 @@ func TestConstraintNotValidDistroVersion(t *testing.T) { result = tc.NotValid(NeedDistroVersionEquals(distroVersion)) assert.False(result) + result = tc.NotValid(NeedDistroVersionLE(higherVersion)) + assert.False(result) + + result = tc.NotValid(NeedDistroVersionLE(distroVersion)) + assert.False(result) + result = tc.NotValid(NeedDistroVersionLT(higherVersion)) assert.False(result) result = tc.NotValid(NeedDistroVersionLT(distroVersion)) assert.True(result) + result = tc.NotValid(NeedDistroVersionGE(higherVersion)) + assert.True(result) + + result = tc.NotValid(NeedDistroVersionGE(distroVersion)) + assert.False(result) + result = tc.NotValid(NeedDistroVersionGT(higherVersion)) assert.True(result) result = tc.NotValid(NeedDistroVersionGT(distroVersion)) assert.True(result) + + result = tc.NotValid(NeedDistroVersionNotEquals(higherVersion)) + assert.False(result) + + result = tc.NotValid(NeedDistroVersionNotEquals(distroVersion)) + assert.True(result) } }