Skip to content

Commit 15d7023

Browse files
athomascommit-bot@chromium.org
athomas
authored and
[email protected]
committed
[testing] Fix name expansion for service tests with multiple VMOptions
This fixes an issue introduced when DDS testing support was added to the test suite. Previously, tests with multiple VMOptions lines would get the same name for both DDS and service variants. This caused incorrect behavior in the results processing that broke deflaking, but also caused one of the results to be ignored by the infrastructure. Also adds some tests for VMOptions handling in test_suite.dart. Fixes #43768 Change-Id: I3c0f9cbc1807fe814aed5ecb7531ef4289e95683 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/166858 Reviewed-by: William Hesse <[email protected]> Commit-Queue: Alexander Thomas <[email protected]>
1 parent b3ae203 commit 15d7023

File tree

3 files changed

+81
-40
lines changed

3 files changed

+81
-40
lines changed

‎pkg/test_runner/lib/src/test_suite.dart‎

Lines changed: 12 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -600,14 +600,12 @@ class StandardTestSuite extends TestSuite {
600600
for (var vmOptionsVariant = 0;
601601
vmOptionsVariant < vmOptionsList.length;
602602
vmOptionsVariant++) {
603-
var vmOptions = vmOptionsList[vmOptionsVariant];
604-
var allVmOptions = vmOptions;
605-
if (extraVmOptions.isNotEmpty) {
606-
allVmOptions = vmOptions.toList()..addAll(extraVmOptions);
607-
}
608-
603+
var vmOptions = [
604+
...vmOptionsList[vmOptionsVariant],
605+
...extraVmOptions,
606+
];
609607
var isCrashExpected = expectations.contains(Expectation.crash);
610-
var commands = _makeCommands(testFile, vmOptionsVariant, allVmOptions,
608+
var commands = _makeCommands(testFile, vmOptionsVariant, vmOptions,
611609
commonArguments, isCrashExpected);
612610
var variantTestName = testFile.name;
613611
if (vmOptionsList.length > 1) {
@@ -630,25 +628,22 @@ class StandardTestSuite extends TestSuite {
630628
for (var vmOptionsVariant = 0;
631629
vmOptionsVariant < vmOptionsList.length;
632630
vmOptionsVariant++) {
633-
var vmOptions = vmOptionsList[vmOptionsVariant];
634-
var allVmOptions = vmOptions;
635-
if (extraVmOptions.isNotEmpty) {
636-
allVmOptions = vmOptions.toList()..addAll(extraVmOptions);
637-
}
638-
if (emitDdsTest) {
639-
allVmOptions.add('-DUSE_DDS=true');
640-
}
631+
var vmOptions = [
632+
...vmOptionsList[vmOptionsVariant],
633+
...extraVmOptions,
634+
if (emitDdsTest) '-DUSE_DDS=true',
635+
];
641636
var isCrashExpected = expectations.contains(Expectation.crash);
642637
var commands = _makeCommands(
643638
testFile,
644639
vmOptionsVariant + (vmOptionsList.length * i),
645-
allVmOptions,
640+
vmOptions,
646641
commonArguments,
647642
isCrashExpected);
648643
var variantTestName =
649644
testFile.name + '/${emitDdsTest ? 'dds' : 'service'}';
650645
if (vmOptionsList.length > 1) {
651-
variantTestName = "${testFile.name}_$vmOptionsVariant";
646+
variantTestName = "${variantTestName}_$vmOptionsVariant";
652647
}
653648

654649
_addTestCase(testFile, variantTestName, commands, expectations, onTest);

‎pkg/test_runner/test/test_suite_test.dart‎

Lines changed: 56 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@ import 'utils.dart';
99

1010
void main() {
1111
testNnbdRequirements();
12+
testVmOptions();
13+
testServiceTestVmOptions();
1214
}
1315

1416
void testNnbdRequirements() {
@@ -23,31 +25,74 @@ void testNnbdRequirements() {
2325
];
2426

2527
expectTestCases(
26-
[], testFiles, ["language_2/none_test", "language_2/legacy_test"]);
28+
[], testFiles, ["language/none_test", "language/legacy_test"]);
2729

2830
expectTestCases(["--nnbd=legacy"], testFiles,
29-
["language_2/none_test", "language_2/legacy_test"]);
31+
["language/none_test", "language/legacy_test"]);
3032

3133
expectTestCases(["--nnbd=weak"], testFiles,
32-
["language_2/none_test", "language_2/nnbd_test", "language_2/weak_test"]);
34+
["language/none_test", "language/nnbd_test", "language/weak_test"]);
35+
36+
expectTestCases(["--nnbd=strong"], testFiles,
37+
["language/none_test", "language/nnbd_test", "language/strong_test"]);
38+
}
39+
40+
void testVmOptions() {
41+
// Note: The backslashes are to avoid the test_runner thinking these are
42+
// Requirements markers for this file itself.
43+
var testFiles = [
44+
parseTestFile("", path: "vm_no_options_test.dart"),
45+
parseTestFile("/\/ VMOptions=--a", path: "vm_one_option_test.dart"),
46+
parseTestFile("/\/ VMOptions=--a --b\n/\/ VMOptions=--c",
47+
path: "vm_options_test.dart"),
48+
];
3349

3450
expectTestCases(
35-
["--nnbd=strong"],
51+
[],
3652
testFiles,
3753
[
38-
"language_2/none_test",
39-
"language_2/nnbd_test",
40-
"language_2/strong_test"
54+
"language/vm_no_options_test",
55+
"language/vm_one_option_test",
56+
"language/vm_options_test/0",
57+
"language/vm_options_test/1",
4158
]);
4259
}
4360

61+
void testServiceTestVmOptions() {
62+
// Note: The backslashes are to avoid the test_runner thinking these are
63+
// Requirements markers for this file itself.
64+
var testFiles = [
65+
parseTestFile("", path: "service_no_options_test.dart", suite: "service"),
66+
parseTestFile("/\/ VMOptions=--a",
67+
path: "service_one_option_test.dart", suite: "service"),
68+
parseTestFile("/\/ VMOptions=--a --b\n/\/ VMOptions=--c",
69+
path: "service_options_test.dart", suite: "service"),
70+
];
71+
72+
expectTestCases(
73+
[],
74+
testFiles,
75+
[
76+
"service/service_no_options_test/service",
77+
"service/service_no_options_test/dds",
78+
"service/service_one_option_test/service",
79+
"service/service_one_option_test/dds",
80+
"service/service_options_test/service_0",
81+
"service/service_options_test/service_1",
82+
"service/service_options_test/dds_0",
83+
"service/service_options_test/dds_1",
84+
],
85+
suite: "service");
86+
}
87+
4488
void expectTestCases(List<String> options, List<TestFile> testFiles,
45-
List<String> expectedCaseNames) {
46-
var configuration = makeConfiguration(options);
47-
var suite = makeTestSuite(configuration, testFiles);
89+
List<String> expectedCaseNames,
90+
{String suite = "language"}) {
91+
var configuration = makeConfiguration(options, suite);
92+
var testSuite = makeTestSuite(configuration, testFiles, suite);
4893

4994
var testCaseNames = <String>[];
50-
suite.findTestCases((testCase) {
95+
testSuite.findTestCases((testCase) {
5196
testCaseNames.add(testCase.displayName);
5297
}, {});
5398

‎pkg/test_runner/test/utils.dart‎

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -8,22 +8,23 @@ import 'package:test_runner/src/static_error.dart';
88
import 'package:test_runner/src/test_file.dart';
99
import 'package:test_runner/src/test_suite.dart';
1010

11-
final Path _suiteDirectory = Path("language_2");
12-
13-
TestFile parseTestFile(String source, {String path = "some_test.dart"}) {
14-
path = _suiteDirectory.absolute.append(path).toNativePath();
15-
return TestFile.parse(_suiteDirectory.absolute, path, source);
11+
TestFile parseTestFile(String source,
12+
{String path = "some_test.dart", String suite = "language"}) {
13+
final suiteDirectory = Path(suite);
14+
path = suiteDirectory.absolute.append(path).toNativePath();
15+
return TestFile.parse(suiteDirectory.absolute, path, source);
1616
}
1717

1818
// TODO(rnystrom): Would be nice if there was a simpler way to create a
1919
// configuration for use in unit tests.
20-
TestConfiguration makeConfiguration(List<String> arguments) =>
21-
OptionsParser().parse([...arguments, "language_2"]).first;
20+
TestConfiguration makeConfiguration(List<String> arguments, String suite) {
21+
return OptionsParser().parse([...arguments, suite]).first;
22+
}
2223

2324
/// Creates a [StandardTestSuite] hardcoded to contain [testFiles].
24-
StandardTestSuite makeTestSuite(
25-
TestConfiguration configuration, List<TestFile> testFiles) =>
26-
_MockTestSuite(configuration, testFiles);
25+
StandardTestSuite makeTestSuite(TestConfiguration configuration,
26+
List<TestFile> testFiles, String suite) =>
27+
_MockTestSuite(configuration, testFiles, suite);
2728

2829
StaticError makeError(
2930
{int line = 1,
@@ -43,8 +44,8 @@ StaticError makeError(
4344
class _MockTestSuite extends StandardTestSuite {
4445
final List<TestFile> _testFiles;
4546

46-
_MockTestSuite(TestConfiguration configuration, this._testFiles)
47-
: super(configuration, "language_2", _suiteDirectory, []);
47+
_MockTestSuite(TestConfiguration configuration, this._testFiles, String suite)
48+
: super(configuration, suite, Path(suite), []);
4849

4950
@override
5051
Iterable<TestFile> findTests() => _testFiles;

0 commit comments

Comments
 (0)