Skip to content

Commit fd10ba9

Browse files
ivanceaalbertzaharovits
ivancea
authored and
albertzaharovits
committed
Fix broken "How to reproduce" lines in CSV tests (#110711)
Fixes #105017 A quick summary: - `randomizedtesting` library doesn't handle "+" chars in test names - We currently have the "...#[skip:..., ...]" instructions as part of the test name As removing the instructions from the name fixes both issues (And makes launching a single test in CLI far easier), the instructions have been moved out of the test name to another parameter. Before: ``` gradlew ':x-pack:plugin:esql:test' --tests "org.elasticsearch.xpack.esql.CsvTests" -Dtests.method="test {stats.GroupByNull#[skip:-8.12.99,reason:bug fixed in 8.13+]}" ``` After: ``` gradlew ":x-pack:plugin:esql:test" --tests "org.elasticsearch.xpack.esql.CsvTests" -Dtests.method="test {stats.GroupByNull}" ```
1 parent f9cae46 commit fd10ba9

File tree

9 files changed

+86
-23
lines changed

9 files changed

+86
-23
lines changed

‎test/framework/src/main/java/org/elasticsearch/test/junit/listeners/ReproduceInfoPrinter.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ public void testFailure(Failure failure) throws Exception {
6767
boolean isBwcTest = Boolean.parseBoolean(System.getProperty("tests.bwc", "false"));
6868

6969
// append Gradle test runner test filter string
70-
b.append("'" + task + "'");
70+
b.append("\"" + task + "\"");
7171
if (isBwcTest) {
7272
// Use "legacy" method for bwc tests so that it applies globally to all upstream bwc test tasks
7373
b.append(" -Dtests.class=\"");

‎x-pack/plugin/esql-core/test-fixtures/src/main/java/org/elasticsearch/xpack/esql/core/SpecReader.java

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ public static List<Object[]> readURLSpec(URL source, Parser parser) throws Excep
7979
Object result = parser.parse(line);
8080
// only if the parser is ready, add the object - otherwise keep on serving it lines
8181
if (result != null) {
82-
testCases.add(new Object[] { fileName, groupName, testName, Integer.valueOf(lineNumber), result });
82+
testCases.add(makeTestCase(fileName, groupName, testName, lineNumber, result));
8383
testName = null;
8484
}
8585
}
@@ -102,4 +102,13 @@ public interface Parser {
102102
public static boolean shouldSkipLine(String line) {
103103
return line.isEmpty() || line.startsWith("//") || line.startsWith("#");
104104
}
105+
106+
private static Object[] makeTestCase(String fileName, String groupName, String testName, int lineNumber, Object result) {
107+
var testNameParts = testName.split("#", 2);
108+
109+
testName = testNameParts[0];
110+
var instructions = testNameParts.length == 2 ? testNameParts[1] : "";
111+
112+
return new Object[] { fileName, groupName, testName, lineNumber, result, instructions };
113+
}
105114
}

‎x-pack/plugin/esql/qa/server/mixed-cluster/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/mixed/MixedClusterEsqlSpecIT.java

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -56,14 +56,22 @@ public static void cleanUp() {
5656
oldClusterTestFeatureService = null;
5757
}
5858

59-
public MixedClusterEsqlSpecIT(String fileName, String groupName, String testName, Integer lineNumber, CsvTestCase testCase, Mode mode) {
60-
super(fileName, groupName, testName, lineNumber, testCase, mode);
59+
public MixedClusterEsqlSpecIT(
60+
String fileName,
61+
String groupName,
62+
String testName,
63+
Integer lineNumber,
64+
CsvTestCase testCase,
65+
String instructions,
66+
Mode mode
67+
) {
68+
super(fileName, groupName, testName, lineNumber, testCase, instructions, mode);
6169
}
6270

6371
@Override
6472
protected void shouldSkipTest(String testName) throws IOException {
6573
super.shouldSkipTest(testName);
66-
assumeTrue("Test " + testName + " is skipped on " + bwcVersion, isEnabled(testName, bwcVersion));
74+
assumeTrue("Test " + testName + " is skipped on " + bwcVersion, isEnabled(testName, instructions, bwcVersion));
6775
if (mode == ASYNC) {
6876
assumeTrue("Async is not supported on " + bwcVersion, supportsAsync());
6977
}

‎x-pack/plugin/esql/qa/server/multi-clusters/src/javaRestTest/java/org/elasticsearch/xpack/esql/ccq/MultiClusterSpecIT.java

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -88,16 +88,27 @@ public static List<Object[]> readScriptSpec() throws Exception {
8888
return testcases;
8989
}
9090

91-
public MultiClusterSpecIT(String fileName, String groupName, String testName, Integer lineNumber, CsvTestCase testCase, Mode mode) {
92-
super(fileName, groupName, testName, lineNumber, convertToRemoteIndices(testCase), mode);
91+
public MultiClusterSpecIT(
92+
String fileName,
93+
String groupName,
94+
String testName,
95+
Integer lineNumber,
96+
CsvTestCase testCase,
97+
String instructions,
98+
Mode mode
99+
) {
100+
super(fileName, groupName, testName, lineNumber, convertToRemoteIndices(testCase), instructions, mode);
93101
}
94102

95103
@Override
96104
protected void shouldSkipTest(String testName) throws IOException {
97105
super.shouldSkipTest(testName);
98106
checkCapabilities(remoteClusterClient(), remoteFeaturesService(), testName, testCase);
99107
assumeFalse("can't test with _index metadata", hasIndexMetadata(testCase.query));
100-
assumeTrue("Test " + testName + " is skipped on " + Clusters.oldVersion(), isEnabled(testName, Clusters.oldVersion()));
108+
assumeTrue(
109+
"Test " + testName + " is skipped on " + Clusters.oldVersion(),
110+
isEnabled(testName, instructions, Clusters.oldVersion())
111+
);
101112
}
102113

103114
private TestFeatureService remoteFeaturesService() throws IOException {

‎x-pack/plugin/esql/qa/server/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/multi_node/EsqlSpecIT.java

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,16 @@ protected String getTestRestCluster() {
2121
return cluster.getHttpAddresses();
2222
}
2323

24-
public EsqlSpecIT(String fileName, String groupName, String testName, Integer lineNumber, CsvTestCase testCase, Mode mode) {
25-
super(fileName, groupName, testName, lineNumber, testCase, mode);
24+
public EsqlSpecIT(
25+
String fileName,
26+
String groupName,
27+
String testName,
28+
Integer lineNumber,
29+
CsvTestCase testCase,
30+
String instructions,
31+
Mode mode
32+
) {
33+
super(fileName, groupName, testName, lineNumber, testCase, instructions, mode);
2634
}
2735

2836
@Override

‎x-pack/plugin/esql/qa/server/single-node/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/single_node/EsqlSpecIT.java

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,16 @@ protected String getTestRestCluster() {
2525
return cluster.getHttpAddresses();
2626
}
2727

28-
public EsqlSpecIT(String fileName, String groupName, String testName, Integer lineNumber, CsvTestCase testCase, Mode mode) {
29-
super(fileName, groupName, testName, lineNumber, testCase, mode);
28+
public EsqlSpecIT(
29+
String fileName,
30+
String groupName,
31+
String testName,
32+
Integer lineNumber,
33+
CsvTestCase testCase,
34+
String instructions,
35+
Mode mode
36+
) {
37+
super(fileName, groupName, testName, lineNumber, testCase, instructions, mode);
3038
}
3139

3240
@Override

‎x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/EsqlSpecTestCase.java

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,7 @@ public abstract class EsqlSpecTestCase extends ESRestTestCase {
8282
private final String testName;
8383
private final Integer lineNumber;
8484
protected final CsvTestCase testCase;
85+
protected final String instructions;
8586
protected final Mode mode;
8687

8788
public enum Mode {
@@ -108,12 +109,21 @@ public static List<Object[]> readScriptSpec() throws Exception {
108109
return testcases;
109110
}
110111

111-
protected EsqlSpecTestCase(String fileName, String groupName, String testName, Integer lineNumber, CsvTestCase testCase, Mode mode) {
112+
protected EsqlSpecTestCase(
113+
String fileName,
114+
String groupName,
115+
String testName,
116+
Integer lineNumber,
117+
CsvTestCase testCase,
118+
String instructions,
119+
Mode mode
120+
) {
112121
this.fileName = fileName;
113122
this.groupName = groupName;
114123
this.testName = testName;
115124
this.lineNumber = lineNumber;
116125
this.testCase = testCase;
126+
this.instructions = instructions;
117127
this.mode = mode;
118128
}
119129

@@ -155,7 +165,7 @@ public final void test() throws Throwable {
155165

156166
protected void shouldSkipTest(String testName) throws IOException {
157167
checkCapabilities(adminClient(), testFeatureService, testName, testCase);
158-
assumeTrue("Test " + testName + " is not enabled", isEnabled(testName, Version.CURRENT));
168+
assumeTrue("Test " + testName + " is not enabled", isEnabled(testName, instructions, Version.CURRENT));
159169
}
160170

161171
protected static void checkCapabilities(RestClient client, TestFeatureService testFeatureService, String testName, CsvTestCase testCase)

‎x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/CsvTestUtils.java

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -69,21 +69,21 @@ public final class CsvTestUtils {
6969

7070
private CsvTestUtils() {}
7171

72-
public static boolean isEnabled(String testName, Version version) {
72+
public static boolean isEnabled(String testName, String instructions, Version version) {
7373
if (testName.endsWith("-Ignore")) {
7474
return false;
7575
}
76-
Tuple<Version, Version> skipRange = skipVersionRange(testName);
76+
Tuple<Version, Version> skipRange = skipVersionRange(testName, instructions);
7777
if (skipRange != null && version.onOrAfter(skipRange.v1()) && version.onOrBefore(skipRange.v2())) {
7878
return false;
7979
}
8080
return true;
8181
}
8282

83-
private static final Pattern INSTRUCTION_PATTERN = Pattern.compile("#\\[(.*?)]");
83+
private static final Pattern INSTRUCTION_PATTERN = Pattern.compile("\\[(.*?)]");
8484

85-
public static Map<String, String> extractInstructions(String testName) {
86-
Matcher matcher = INSTRUCTION_PATTERN.matcher(testName);
85+
public static Map<String, String> parseInstructions(String instructions) {
86+
Matcher matcher = INSTRUCTION_PATTERN.matcher(instructions);
8787
Map<String, String> pairs = new HashMap<>();
8888
if (matcher.find()) {
8989
String[] groups = matcher.group(1).split(",");
@@ -98,8 +98,8 @@ public static Map<String, String> extractInstructions(String testName) {
9898
return pairs;
9999
}
100100

101-
public static Tuple<Version, Version> skipVersionRange(String testName) {
102-
Map<String, String> pairs = extractInstructions(testName);
101+
public static Tuple<Version, Version> skipVersionRange(String testName, String instructions) {
102+
Map<String, String> pairs = parseInstructions(instructions);
103103
String versionRange = pairs.get("skip");
104104
if (versionRange != null) {
105105
String[] skipVersions = versionRange.split("-", Integer.MAX_VALUE);

‎x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/CsvTests.java

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,7 @@ public class CsvTests extends ESTestCase {
157157
private final String testName;
158158
private final Integer lineNumber;
159159
private final CsvSpecReader.CsvTestCase testCase;
160+
private final String instructions;
160161

161162
private final EsqlConfiguration configuration = EsqlTestUtils.configuration(
162163
new QueryPragmas(Settings.builder().put("page_size", randomPageSize()).build())
@@ -211,17 +212,25 @@ private int randomPageSize() {
211212
}
212213
}
213214

214-
public CsvTests(String fileName, String groupName, String testName, Integer lineNumber, CsvSpecReader.CsvTestCase testCase) {
215+
public CsvTests(
216+
String fileName,
217+
String groupName,
218+
String testName,
219+
Integer lineNumber,
220+
CsvSpecReader.CsvTestCase testCase,
221+
String instructions
222+
) {
215223
this.fileName = fileName;
216224
this.groupName = groupName;
217225
this.testName = testName;
218226
this.lineNumber = lineNumber;
219227
this.testCase = testCase;
228+
this.instructions = instructions;
220229
}
221230

222231
public final void test() throws Throwable {
223232
try {
224-
assumeTrue("Test " + testName + " is not enabled", isEnabled(testName, Version.CURRENT));
233+
assumeTrue("Test " + testName + " is not enabled", isEnabled(testName, instructions, Version.CURRENT));
225234
/*
226235
* The csv tests support all but a few features. The unsupported features
227236
* are tested in integration tests.

0 commit comments

Comments
 (0)