Skip to content

Commit

Permalink
roachtest: direct mixedversion planning errors to test-eng
Browse files Browse the repository at this point in the history
Mixedversion planning failures are always internal errors and
therefore shouldn't lead to issues being assigned to the team that
owns the test. We fix that in this commit.

Epic: none

Release note: None
  • Loading branch information
renatolabs committed Jul 22, 2024
1 parent 1d0b703 commit 15d9f5b
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 3 deletions.
16 changes: 14 additions & 2 deletions pkg/cmd/roachtest/roachtestutil/mixedversion/mixedversion.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ import (

"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/cluster"
"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/option"
"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/registry"
"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/roachtestutil"
"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/roachtestutil/clusterupgrade"
"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/test"
Expand Down Expand Up @@ -636,7 +637,7 @@ func (t *Test) Workload(
func (t *Test) Run() {
plan, err := t.plan()
if err != nil {
t.rt.Fatal(fmt.Errorf("error creating test plan: %w", err))
t.rt.Fatal(err)
}

t.logger.Printf("mixed-version test:\n%s", plan.PrettyPrint())
Expand All @@ -652,7 +653,18 @@ func (t *Test) run(plan *TestPlan) error {
).run()
}

func (t *Test) plan() (*TestPlan, error) {
func (t *Test) plan() (plan *TestPlan, retErr error) {
defer func() {
if retErr != nil {
// Planning failures are always internal framework errors, so
// they should be sent to TestEng.
retErr = registry.ErrorWithOwner(
registry.OwnerTestEng,
fmt.Errorf("error creating test plan: %w", retErr),
)
}
}()

previousReleases, err := t.choosePreviousReleases()
if err != nil {
return nil, err
Expand Down
19 changes: 19 additions & 0 deletions pkg/cmd/roachtest/roachtestutil/mixedversion/mixedversion_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,12 @@ import (
"testing"

"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/option"
"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/registry"
"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/roachtestutil/clusterupgrade"
"github.com/cockroachdb/cockroach/pkg/roachprod/vm"
"github.com/cockroachdb/cockroach/pkg/testutils/release"
"github.com/cockroachdb/cockroach/pkg/util/version"
"github.com/cockroachdb/errors"
"github.com/stretchr/testify/require"
)

Expand Down Expand Up @@ -192,6 +194,23 @@ func Test_choosePreviousReleases(t *testing.T) {
}
}

func TestTest_plan(t *testing.T) {
// Assert that planning failures are owned by test-eng. At the time
// of writing, planning can only return an error if we fail to find
// a required predecessor of a certain release.
mvt := newTest(NumUpgrades(100))
_, err := mvt.plan()
require.Error(t, err)
require.Contains(t, err.Error(), "no known predecessor for")

var errWithOwnership registry.ErrorWithOwnership
require.True(t,
errors.As(err, &errWithOwnership),
"error %q (%T) is not ErrorWithOwnership", err.Error(), err,
)
require.Equal(t, registry.OwnerTestEng, errWithOwnership.Owner)
}

// withTestBuildVersion overwrites the `TestBuildVersion` variable in
// the `clusterupgrade` package, allowing tests to set a fixed
// "current version". Returns a function that resets that variable.
Expand Down
7 changes: 6 additions & 1 deletion pkg/cmd/roachtest/roachtestutil/mixedversion/planner_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -375,7 +375,12 @@ func boolP(b bool) *bool {
func testPredecessorFunc(
rng *rand.Rand, v *clusterupgrade.Version,
) (*clusterupgrade.Version, error) {
return testPredecessorMapping[release.VersionSeries(&v.Version)], nil
pred, ok := testPredecessorMapping[release.VersionSeries(&v.Version)]
if !ok {
return nil, fmt.Errorf("no known predecessor for %q", v)
}

return pred, nil
}

// createDataDrivenMixedVersionTest creates a `*Test` instance based
Expand Down

0 comments on commit 15d9f5b

Please sign in to comment.