Skip to content

Commit

Permalink
Merge cockroachdb#135501
Browse files Browse the repository at this point in the history 
135501: logictest: allow multiple configs for skipif, onlyif r=RaduBerinde a=RaduBerinde

This is mostly a convenience for `skipif` but it is a necessity for
`onlyif`, which we can't use twice.

Epic: none
Release note: None

Co-authored-by: Radu Berinde <[email protected]>
  • Loading branch information
@RaduBerinde
craig[bot] and RaduBerinde committed Nov 18, 2024
2 parents 864b6c3 + a534f77 commit 3397bc5
Show file tree
Hide file tree
Showing 18 changed files with 152 additions and 137 deletions.
5 changes: 4 additions & 1 deletion pkg/sql/logictest/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,10 @@ filegroup(
go_library(
name = "logictest",
testonly = 1,
srcs = ["logic.go"],
srcs = [
"logic.go",
"parsing.go",
],
importpath = "github.com/cockroachdb/cockroach/pkg/sql/logictest",
visibility = ["//visibility:public"],
deps = [
Expand Down
110 changes: 52 additions & 58 deletions pkg/sql/logictest/logic.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -392,25 +392,25 @@ import (
// When using a cockroach-go/testserver logictest, upgrades the node at
// index N to the version specified by the logictest config.
//
// - skip <ISSUE> [args...]
// - skip #ISSUE [args...]
// Skips this entire logic test using skip.WithIssue(). Should be near top of
// test file. Note that this is different from `skipif`.
//
// - skip ignorelint [args...]
// Skips this entire logic test using skip.IgnoreLint(). Should be near top
// of test file. Note that this is different from `skipif`.
//
// - skip under <deadlock/race/stress/metamorphic/duress> [ISSUE] [args...]
// - skip under <deadlock/race/stress/metamorphic/duress> [#ISSUE] [args...]
// Skips this entire logic test using skip.UnderDeadlock(), skip.UnderRace(),
// etc. Should be near top of test file. Note that this is different from
// `skipif`.
//
// - skipif <mysql/mssql/postgresql/cockroachdb/config CONFIG [ISSUE]>
// - skipif <mysql/mssql/postgresql/cockroachdb/config [#ISSUE] CONFIG [CONFIG...]
// Skips the following `statement` or `query` if the argument is postgresql,
// cockroachdb, or a config matching the currently running
// configuration. Note that this is different from `skip`.
//
// - onlyif <mysql/mssql/postgresql/cockroachdb/config CONFIG [ISSUE]>
// - onlyif <mysql/mssql/postgresql/cockroachdb/config [#ISSUE] CONFIG [CONFIG...]
// Skips the following `statement` or `query` if the argument is not
// postgresql, cockroachdb, or a config matching the currently
// running configuration.
Expand Down Expand Up @@ -2538,6 +2538,8 @@ func (t *logicTest) processSubtest(
repeat := 1
t.retry = false

// onlyIfConfig is used to disallow multiple "onlyif config" lines.
onlyIfConfig := false
for s.Scan() {
t.curPath, t.curLineNo = path, s.Line+subtest.lineLineIndexIntoFile
if *maxErrs > 0 && t.failures >= *maxErrs {
Expand Down Expand Up @@ -2644,6 +2646,7 @@ func (t *logicTest) processSubtest(
// command.
t.retry = true
case "statement":
onlyIfConfig = false
stmt := logicStatement{
pos: fmt.Sprintf("\n%s:%d", path, s.Line+subtest.lineLineIndexIntoFile),
expectCount: -1,
Expand Down Expand Up @@ -2721,6 +2724,7 @@ func (t *logicTest) processSubtest(
t.success(path)

case "query":
onlyIfConfig = false
var query logicQuery
query.pos = fmt.Sprintf("\n%s:%d", path, s.Line+subtest.lineLineIndexIntoFile)
query.nodeIdx = t.nodeIdx
Expand Down Expand Up @@ -3136,68 +3140,49 @@ func (t *logicTest) processSubtest(
return errors.Errorf("skip requires an argument")
}

// Parse [ISSUE] [args...] as the trailing arguments for most skip
// commands. Returns -1 if the first field is not parsable as a GitHub
// issue number.
parse := func(fields []string) (int, []interface{}) {
if len(fields) < 1 {
return -1, nil
}
if githubIssueID, err := strconv.ParseUint(fields[0], 10, 32); err == nil {
args := make([]interface{}, len(fields)-1)
for i := range args {
args[i] = fields[i+1]
}
return int(githubIssueID), args
}
args := make([]interface{}, len(fields))
for i := range args {
args[i] = fields[i]
}
return -1, args
}

switch fields[1] {
case "ignorelint":
if githubIssueID, args := parse(fields[2:]); githubIssueID < 0 {
skip.IgnoreLint(t.t(), args...)
if githubIssueID, args := extractGithubIssue(fields[2:]); githubIssueID < 0 {
skip.IgnoreLint(t.t(), strings.Join(args, " "))
} else {
return errors.Errorf("skip ignorelint does not take an issue ID: %v", githubIssueID)
}
case "under":
if len(fields) < 3 || fields[2] == "" {
return errors.Errorf("skip under command requires an argument")
}
githubIssueID, args := extractGithubIssue(fields[3:])
msg := strings.Join(args, " ")
switch fields[2] {
case "deadlock":
if githubIssueID, args := parse(fields[3:]); githubIssueID < 0 {
skip.UnderDeadlock(t.t(), args...)
if githubIssueID < 0 {
skip.UnderDeadlock(t.t(), msg)
} else {
skip.UnderDeadlockWithIssue(t.t(), githubIssueID, args...)
skip.UnderDeadlockWithIssue(t.t(), githubIssueID, msg)
}
case "race":
if githubIssueID, args := parse(fields[3:]); githubIssueID < 0 {
skip.UnderRace(t.t(), args...)
if githubIssueID < 0 {
skip.UnderRace(t.t(), msg)
} else {
skip.UnderRaceWithIssue(t.t(), githubIssueID, args...)
skip.UnderRaceWithIssue(t.t(), githubIssueID, msg)
}
case "stress":
if githubIssueID, args := parse(fields[3:]); githubIssueID < 0 {
skip.UnderStress(t.t(), args...)
if githubIssueID < 0 {
skip.UnderStress(t.t(), msg)
} else {
skip.UnderStressWithIssue(t.t(), githubIssueID, args...)
skip.UnderStressWithIssue(t.t(), githubIssueID, msg)
}
case "metamorphic":
if githubIssueID, args := parse(fields[3:]); githubIssueID < 0 {
skip.UnderMetamorphic(t.t(), args...)
if githubIssueID < 0 {
skip.UnderMetamorphic(t.t(), msg)
} else {
skip.UnderMetamorphicWithIssue(t.t(), githubIssueID, args...)
skip.UnderMetamorphicWithIssue(t.t(), githubIssueID, msg)
}
case "duress":
if githubIssueID, args := parse(fields[3:]); githubIssueID < 0 {
skip.UnderDuress(t.t(), args...)
if githubIssueID < 0 {
skip.UnderDuress(t.t(), msg)
} else {
skip.UnderDuressWithIssue(t.t(), githubIssueID, args...)
skip.UnderDuressWithIssue(t.t(), githubIssueID, msg)
}
default:
return errors.Errorf("unsupported skip under command: %v", fields[2])
Expand All @@ -3208,11 +3193,11 @@ func (t *logicTest) processSubtest(
path, s.Line+subtest.lineLineIndexIntoFile,
)
default:
githubIssueID, args := parse(fields[1:])
githubIssueID, args := extractGithubIssue(fields[1:])
if githubIssueID < 0 {
return errors.Errorf("unsupported skip command: %v", fields[1])
}
skip.WithIssue(t.t(), githubIssueID, args...)
skip.WithIssue(t.t(), githubIssueID, strings.Join(args, " "))
}

case "force-backup-restore":
Expand All @@ -3232,15 +3217,14 @@ func (t *logicTest) processSubtest(
continue
case "config":
if len(fields) < 3 {
return errors.New("skipif config CONFIG [ISSUE] command requires configuration parameter")
return errors.New("skipif config [#ISSUE] CONFIG [CONFIG...] missing argument")
}
configName := fields[2]
if t.cfg.Name == configName || logictestbase.ConfigIsInDefaultList(t.cfg.Name, configName) {
issue := "no issue given"
if len(fields) > 3 {
issue = fields[3]
githubIssueID, args := extractGithubIssue(fields[2:])
for _, configName := range args {
if t.cfg.Name == configName || logictestbase.ConfigIsInDefaultList(t.cfg.Name, configName) {
s.SetSkip(fmt.Sprintf("unsupported configuration %s (%s)", configName, githubIssueStr(githubIssueID)))
break
}
s.SetSkip(fmt.Sprintf("unsupported configuration %s (%s)", configName, issue))
}
case "backup-restore":
if config.BackupRestoreProbability > 0.0 {
Expand Down Expand Up @@ -3268,16 +3252,26 @@ func (t *logicTest) processSubtest(
s.SetSkip("")
continue
case "config":
if onlyIfConfig {
return errors.New("multiple onlyif config statements are not allowed")
}
onlyIfConfig = true

if len(fields) < 3 {
return errors.New("onlyif config CONFIG [ISSUE] command requires configuration parameter")
return errors.New("onlyif config [#ISSUE] CONFIG [CONFIG...] missing argument")
}
configName := fields[2]
if t.cfg.Name != configName && !logictestbase.ConfigIsInDefaultList(t.cfg.Name, configName) {
issue := "no issue given"
if len(fields) > 3 {
issue = fields[3]
githubIssueID, args := extractGithubIssue(fields[2:])
shouldSkip := true
for _, configName := range args {
if t.cfg.Name == configName || logictestbase.ConfigIsInDefaultList(t.cfg.Name, configName) {
// Our config matches one item in the list.
shouldSkip = false
break
}
s.SetSkip(fmt.Sprintf("unsupported configuration %s, statement/query only supports %s (%s)", t.cfg.Name, configName, issue))
}
if shouldSkip {
s.SetSkip(fmt.Sprintf("unsupported configuration %s, statement/query only supports %s (%s)",
t.cfg.Name, strings.Join(args, "/"), githubIssueStr(githubIssueID)))
}
continue
default:
Expand Down
38 changes: 38 additions & 0 deletions pkg/sql/logictest/parsing.go
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// Copyright 2024 The Cockroach Authors.
//
// Use of this software is governed by the CockroachDB Software License
// included in the /LICENSE file.

package logictest

import (
"fmt"
"strconv"
"strings"
)

// extractGithubIssue parses directive fields of the form:
//
// [#ISSUE] [args...]
//
// If the first field is a number prefixed with #, it will be interpreted as
// a github issue and the rest of the fiels are returned. Otherwise, returns -1
// and the given fields.
func extractGithubIssue(fields []string) (githubIssueID int, args []string) {
if len(fields) < 1 {
return -1, nil
}
if numStr, ok := strings.CutPrefix(fields[0], "#"); ok {
if num, err := strconv.ParseUint(numStr, 10, 32); err == nil {
return int(num), fields[1:]
}
}
return -1, fields
}

func githubIssueStr(githubIssueID int) string {
if githubIssueID <= 0 {
return "no issue given"
}
return fmt.Sprintf("#%d", githubIssueID)
}
36 changes: 10 additions & 26 deletions pkg/sql/logictest/testdata/logic_test/alter_column_type
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1389,8 +1389,7 @@ SELECT * FROM stored1 ORDER BY A;
statement ok
INSERT INTO stored1 VALUES (2147483648),(2147483647);

skipif config local-legacy-schema-changer
skipif config local-mixed-24.1
skipif config local-legacy-schema-changer local-mixed-24.1
statement error pq: validate check constraint: integer out of range for type int4
ALTER TABLE stored1 ALTER COLUMN COMP1 SET DATA TYPE INT4;

Expand All @@ -1402,13 +1401,11 @@ ALTER TABLE stored1 ALTER COLUMN COMP1 SET DATA TYPE INT4;
statement ok
DELETE FROM stored1 WHERE a = 2147483648;

skipif config local-legacy-schema-changer
skipif config local-mixed-24.1
skipif config local-legacy-schema-changer local-mixed-24.1
statement ok
ALTER TABLE stored1 ALTER COLUMN COMP1 SET DATA TYPE INT4;

skipif config local-legacy-schema-changer
skipif config local-mixed-24.1
skipif config local-legacy-schema-changer local-mixed-24.1
query TT
SHOW CREATE TABLE stored1;
----
Expand All @@ -1419,8 +1416,7 @@ stored1 CREATE TABLE public.stored1 (
FAMILY f1 (a, comp1)
)

skipif config local-legacy-schema-changer
skipif config local-mixed-24.1
skipif config local-legacy-schema-changer local-mixed-24.1
query II
SELECT * FROM stored1 ORDER BY A;
----
Expand All @@ -1430,33 +1426,25 @@ SELECT * FROM stored1 ORDER BY A;
2000 2000
2147483647 2147483647

skipif config local-legacy-schema-changer
skipif config local-mixed-24.1
skipif config local-mixed-24.2
skipif config local-legacy-schema-changer local-mixed-24.1 local-mixed-24.2
# Attempt to convert to a type that is incompatible with the computed expression
statement error pq: expected STORED COMPUTED COLUMN expression to have type bool, but 'a' has type int
ALTER TABLE stored1 ALTER COLUMN comp1 SET DATA TYPE BOOL;

skipif config local-legacy-schema-changer
skipif config local-mixed-24.1
skipif config local-mixed-24.2
skipif config local-legacy-schema-changer local-mixed-24.1 local-mixed-24.2
statement error pq: expected STORED COMPUTED COLUMN expression to have type string, but 'a' has type int
ALTER TABLE stored1 ALTER COLUMN comp1 SET DATA TYPE TEXT;

# Convert the type to something compatible, but specify a custom value for the
# column with the USING expression. This will force a type conversion with a backfill.
skipif config local-legacy-schema-changer
skipif config local-mixed-24.1
skipif config local-mixed-24.2
skipif config local-legacy-schema-changer local-mixed-24.1 local-mixed-24.2
statement ok
ALTER TABLE stored1 ALTER COLUMN comp1 SET DATA TYPE INT2 USING -1;

statement ok
INSERT INTO stored1 VALUES (-1000);

skipif config local-legacy-schema-changer
skipif config local-mixed-24.1
skipif config local-mixed-24.2
skipif config local-legacy-schema-changer local-mixed-24.1 local-mixed-24.2
query TT
SHOW CREATE TABLE stored1;
----
Expand All @@ -1467,9 +1455,7 @@ stored1 CREATE TABLE public.stored1 (
FAMILY f1 (a, comp1)
)

skipif config local-legacy-schema-changer
skipif config local-mixed-24.1
skipif config local-mixed-24.2
skipif config local-legacy-schema-changer local-mixed-24.1 local-mixed-24.2
query II
SELECT * FROM stored1 ORDER BY A;
----
Expand All @@ -1481,9 +1467,7 @@ SELECT * FROM stored1 ORDER BY A;
2147483647 -1

# Attempt to drop stored along with changing the column type
skipif config local-legacy-schema-changer
skipif config local-mixed-24.1
skipif config local-mixed-24.2
skipif config local-legacy-schema-changer local-mixed-24.1 local-mixed-24.2
statement error pq: unimplemented: ALTER COLUMN TYPE cannot be used in combination with other ALTER TABLE commands
ALTER TABLE stored1 ALTER COLUMN comp1 SET DATA TYPE INT4 USING -1, ALTER COLUMN comp1 drop stored;

Expand Down
Loading

0 comments on commit 3397bc5

Please sign in to comment.