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] 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)