Skip to content

Commit e3b750c

Browse files
mi-acV8 LUCI CQ
mi-ac
authored and
V8 LUCI CQ
committed
[test] Use pathlib in test framework
This turns all paths in the test frameworks and all its suite implementations from strings into pathlib paths. This makes the framework more future-proof, simplifies a lot of code and enhances readability. It comes with the risk of introducing new bugs. This is lowered using an experimental test comparison script: https://crrev.com/c/4648372. Some detailed changes: - The "path" of a test case is now a pathlib path (also in cases where the path doesn't correspond to a location on the file system). This path is always relative to the test root. - The "name" of a test case is the posix string of that path used in status files and at the command line. - All other paths are absolute. - We use some more wrapping with Path() than absolutely necessary. Reducing such a wrapping where it's not needed is a micro-optimization and the potential by introducing a bug by forgetting one is worse. - Some modules are not happy with pathlib objects, hence a few explicit str() conversions are added. Thoughts on risk: - The highest risk is that due to the newly introduced logic, some test cases are silently filtered and not run now. We'll manually compare runs before and after this change. - Each test attempted to run will either pass or fail, failures won't be silent. E.g. a wrong path passed to the cctest executable is flagged as a failure. - Wrongly constructed paths to JS test cases lead to failures (tested several manually). Other failure types: - Some string path not turned into pathlib: Will typically fail when calling any pathlib function. - Some pathlib object treated as a string: Will typically raise an exception, e.g. using in-operator. - Logic bugs in pathlib functions behaving differently than anticipated, e.g. with_suffix(): Will typically result in wrong paths, resulting in errors reading a test case. Though worst case could be silent ignoring. Cq-Include-Trybots: luci.v8.try:v8_numfuzz_dbg Cq-Include-Trybots: luci.v8.try:v8_numfuzz_rel Cq-Include-Trybots: luci.v8.try:v8_numfuzz_tsan_rel Cq-Include-Trybots: luci.v8.try:v8_linux_noi18n_rel Cq-Include-Trybots: luci.v8.try:v8_android_arm64_n5x_rel Cq-Include-Trybots: luci.v8.try:v8_linux64_msan_rel Cq-Include-Trybots: luci.v8.try:v8_linux64_ubsan_rel Cq-Include-Trybots: luci.v8.try:v8_android_arm64_n5x_rel Bug: chromium:1132088 Change-Id: Ifc94feb292174935db969dcf5d174d2eedcf3dfd Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4538761 Reviewed-by: Liviu Rau <[email protected]> Commit-Queue: Michael Achenbach <[email protected]> Cr-Commit-Position: refs/heads/main@{#88502}
1 parent ba82ab2 commit e3b750c

33 files changed

Lines changed: 308 additions & 330 deletions

File tree

‎test/benchmarks/testcfg.py‎

Lines changed: 17 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,7 @@ class TestSuite(testsuite.TestSuite):
101101

102102
def __init__(self, ctx, *args, **kwargs):
103103
super(TestSuite, self).__init__(ctx, *args, **kwargs)
104-
self.testroot = os.path.join(self.root, "data")
104+
self.testroot = self.root / "data"
105105

106106
def _test_loader_class(self):
107107
return TestLoader
@@ -115,24 +115,23 @@ def _get_files_params(self):
115115
path = self.path
116116
testroot = self.suite.testroot
117117
files = []
118-
if path.startswith("kraken"):
119-
files.append(os.path.join(testroot, "%s-data.js" % path))
120-
files.append(os.path.join(testroot, "%s.js" % path))
121-
elif path.startswith("octane"):
122-
files.append(os.path.join(testroot, "octane/base.js"))
123-
files.append(os.path.join(testroot, "%s.js" % path))
124-
if path.startswith("octane/gbemu"):
125-
files.append(os.path.join(testroot, "octane/gbemu-part2.js"))
126-
elif path.startswith("octane/typescript"):
127-
files.append(os.path.join(testroot,
128-
"octane/typescript-compiler.js"))
129-
files.append(os.path.join(testroot, "octane/typescript-input.js"))
130-
elif path.startswith("octane/zlib"):
131-
files.append(os.path.join(testroot, "octane/zlib-data.js"))
118+
if path.parts[0] == "kraken":
119+
files.append(testroot / f"{path}-data.js")
120+
files.append(testroot / f"{path}.js")
121+
elif path.parts[0] == "octane":
122+
files.append(testroot / "octane/base.js")
123+
files.append(testroot / f"{path}.js")
124+
if path.parts[1] == "gbemu":
125+
files.append(testroot / "octane/gbemu-part2.js")
126+
elif path.parts[1] == "typescript":
127+
files.append(testroot / "octane/typescript-compiler.js")
128+
files.append(testroot / "octane/typescript-input.js")
129+
elif path.parts[1] == "zlib":
130+
files.append(testroot / "octane/zlib-data.js")
132131
files += ["-e", "BenchmarkSuite.RunSuites({});"]
133-
elif path.startswith("sunspider"):
134-
files.append(os.path.join(testroot, "%s.js" % path))
132+
elif path.parts[0] == "sunspider":
133+
files.append(self._get_source_path())
135134
return files
136135

137136
def _get_source_path(self):
138-
return os.path.join(self.suite.testroot, self.path + self._get_suffix())
137+
return self.suite.testroot / self.path_js

‎test/bigint/testcfg.py‎

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,7 @@
22
# Use of this source code is governed by a BSD-style license that can be
33
# found in the LICENSE file.
44

5-
# for py2/py3 compatibility
6-
from __future__ import print_function
7-
8-
import os
9-
105
from testrunner.local import command
11-
from testrunner.local import utils
126
from testrunner.local import testsuite
137
from testrunner.objects import testcase
148

@@ -21,13 +15,9 @@ def _get_variants(self, test):
2115

2216
class TestLoader(testsuite.TestLoader):
2317
def _list_test_filenames(self):
24-
shell = os.path.abspath(
25-
os.path.join(self.test_config.shell_dir, SHELL))
26-
if utils.IsWindows():
27-
shell += ".exe"
2818
cmd = self.ctx.command(
2919
cmd_prefix=self.test_config.command_prefix,
30-
shell=shell,
20+
shell=self.test_config.resolve_shell(SHELL),
3121
args=['--list'] + self.test_config.extra_flags)
3222
output = cmd.execute()
3323

@@ -53,7 +43,7 @@ def _test_class(self):
5343

5444
class TestCase(testcase.TestCase):
5545
def _get_files_params(self):
56-
return [self.path]
46+
return [self.name]
5747

5848
def get_shell(self):
5949
return SHELL

‎test/cctest/testcfg.py‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,6 @@
2828
# for py2/py3 compatibility
2929
from __future__ import print_function
3030

31-
import os
3231
import shutil
3332

3433
from testrunner.local import command
@@ -87,4 +86,4 @@ def get_shell(self):
8786
return SHELL
8887

8988
def _get_files_params(self):
90-
return [self.path]
89+
return [self.name]

‎test/debugger/testcfg.py‎

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -45,11 +45,9 @@ def _parse_source_files(self, source):
4545
break
4646

4747
files = []
48-
files.append(os.path.normpath(os.path.join(
49-
self.suite.root, "..", "mjsunit", "mjsunit.js")))
50-
files.append(os.path.join(self.suite.root, "test-api.js"))
51-
files.extend([os.path.normpath(os.path.join(self.suite.root, '..', '..', f))
52-
for f in files_list])
48+
files.append(self.suite.root.parent / "mjsunit" / "mjsunit.js")
49+
files.append(self.suite.root / "test-api.js")
50+
files.extend([self.suite.root.parents[1] / f for f in files_list])
5351
files.append(self._get_source_path())
5452
return files
5553

@@ -66,10 +64,10 @@ def _get_suite_flags(self):
6664
return ['--enable-inspector', '--allow-natives-syntax']
6765

6866
def _get_source_path(self):
69-
base_path = os.path.join(self.suite.root, self.path)
7067
# Try .js first, and fall back to .mjs.
7168
# TODO(v8:9406): clean this up by never separating the path from
7269
# the extension in the first place.
73-
if os.path.exists(base_path + self._get_suffix()):
74-
return base_path + self._get_suffix()
75-
return base_path + '.mjs'
70+
js_file = self.suite.root / self.path_js
71+
if js_file.exists():
72+
return js_file
73+
return self.suite.root / self.path_mjs

‎test/debugging/testcfg.py‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@
77
import shlex
88
import sys
99

10+
from pathlib import Path
11+
1012
from testrunner.local import testsuite
1113
from testrunner.local import utils
1214
from testrunner.objects import testcase
@@ -17,15 +19,15 @@
1719
class PYTestCase(testcase.TestCase):
1820

1921
def get_shell(self):
20-
return os.path.splitext(sys.executable)[0]
22+
return Path(sys.executable).stem
2123

2224
def get_command(self):
2325
return super(PYTestCase, self).get_command()
2426

2527
def _get_cmd_params(self):
2628
return (
2729
self._get_files_params() +
28-
['--', os.path.join(self.test_config.shell_dir, 'd8')] +
30+
['--', self.test_config.shell_dir / 'd8'] +
2931
self._get_source_flags()
3032
)
3133

@@ -70,10 +72,10 @@ def _get_source_flags(self):
7072
return self._source_flags
7173

7274
def _get_source_path(self):
73-
base_path = os.path.join(self.suite.root, self.path)
74-
if os.path.exists(base_path + self._get_suffix()):
75-
return base_path + self._get_suffix()
76-
return base_path + '.py'
75+
js_file = self.suite.root / self.path_js
76+
if js_file.exists():
77+
return js_file
78+
return self.suite.root / self.path_and_suffix('.py')
7779

7880
def skip_predictable(self):
7981
return super(TestCase, self).skip_predictable() or self._expected_fail()

‎test/fuzzer/testcfg.py‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,6 @@
22
# Use of this source code is governed by a BSD-style license that can be
33
# found in the LICENSE file.
44

5-
import os
6-
75
from testrunner.local import testsuite
86
from testrunner.objects import testcase
97

@@ -32,7 +30,7 @@ def test_dirs(self):
3230
return SUB_TESTS
3331

3432
def _to_relpath(self, abspath, _):
35-
return os.path.relpath(abspath, self.suite.root)
33+
return abspath.relative_to(self.suite.root)
3634

3735
def _should_filter_by_name(self, _):
3836
return False
@@ -50,8 +48,7 @@ def _variants_gen_class(self):
5048

5149
class TestCase(testcase.TestCase):
5250
def _get_files_params(self):
53-
suite, name = self.path.split(os.path.sep)
54-
return [os.path.join(self.suite.root, suite, name)]
51+
return [self.suite.root / self.path]
5552

5653
def _get_variant_flags(self):
5754
return []
@@ -63,5 +60,4 @@ def _get_mode_flags(self):
6360
return []
6461

6562
def get_shell(self):
66-
group, _ = self.path.split(os.path.sep, 1)
67-
return 'v8_simple_%s_fuzzer' % group
63+
return f'v8_simple_{self.path.parts[0]}_fuzzer'

‎test/fuzzilli/testcfg.py‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,6 @@
22
# Use of this source code is governed by a BSD-style license that can be
33
# found in the LICENSE file.
44

5-
import os
6-
75
from testrunner.local import testsuite
86
from testrunner.objects import testcase
97

@@ -22,7 +20,7 @@ def test_dirs(self):
2220
return SUB_TESTS
2321

2422
def _to_relpath(self, abspath, _):
25-
return os.path.relpath(abspath, self.suite.root)
23+
return abspath.relative_to(self.suite.root)
2624

2725

2826
class TestSuite(testsuite.TestSuite):
@@ -38,8 +36,7 @@ def _variants_gen_class(self):
3836

3937
class TestCase(testcase.TestCase):
4038
def _get_files_params(self):
41-
suite, name = self.path.split(os.path.sep)
42-
return [os.path.join(self.suite.root, suite, name)]
39+
return [self.suite.root / self.path]
4340

4441
def _get_variant_flags(self):
4542
return []

‎test/inspector/testcfg.py‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -42,30 +42,29 @@ def __init__(self, *args, **kwargs):
4242

4343
def _get_files_params(self):
4444
return [
45-
os.path.join(self.suite.root, PROTOCOL_TEST_JS),
46-
os.path.join(self.suite.root, self.path + self._get_suffix()),
45+
self.suite.root / PROTOCOL_TEST_JS,
46+
self.suite.root / self.path_js,
4747
]
4848

4949
def _get_source_flags(self):
5050
return self._source_flags
5151

5252
def _get_source_path(self):
53-
return os.path.join(self.suite.root, self.path + self._get_suffix())
53+
return self.suite.root / self.path_js
5454

5555
def get_shell(self):
5656
return 'inspector-test'
5757

5858
def get_android_resources(self):
5959
super_resources = super().get_android_resources()
6060
return super_resources + [
61-
os.path.join('test', 'inspector', 'debugger', 'resources',
62-
'break-locations.js'),
63-
os.path.join('test', 'inspector', WASM_INSPECTOR_JS),
61+
self.suite.root /'debugger' /'resources' /'break-locations.js',
62+
self.suite.root / WASM_INSPECTOR_JS,
6463
]
6564

6665
@property
6766
def output_proc(self):
6867
return outproc.ExpectedOutProc(
6968
self.expected_outcomes,
70-
os.path.join(self.suite.root, self.path) + EXPECTED_SUFFIX,
69+
self.suite.root / self.path_and_suffix(EXPECTED_SUFFIX),
7170
self.test_config.regenerate_expected_files)

‎test/intl/testcfg.py‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -73,11 +73,9 @@ def _get_cmd_env(self):
7373

7474
def _get_files_params(self):
7575
files = [
76-
os.path.join(self.suite.root, f) for f in [
77-
'assert.js',
78-
'utils.js',
79-
self.path + self._get_suffix(),
80-
]
76+
self.suite.root / 'assert.js',
77+
self.suite.root / 'utils.js',
78+
self.suite.root / self.path_js,
8179
]
8280

8381
if self.test_config.isolates:
@@ -91,4 +89,4 @@ def _get_suite_flags(self):
9189
return ['--allow-natives-syntax']
9290

9391
def _get_source_path(self):
94-
return os.path.join(self.suite.root, self.path + self._get_suffix())
92+
return self.suite.root / self.path_js

‎test/message/testcfg.py‎

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -49,16 +49,12 @@ class TestCase(testcase.D8TestCase):
4949
def __init__(self, *args, **kwargs):
5050
super(TestCase, self).__init__(*args, **kwargs)
5151

52-
# get_source() relies on this being set.
53-
self._base_path = os.path.join(self.suite.root, self.path)
5452
source = self.get_source()
5553
self._source_files = self._parse_source_files(source)
5654
self._source_flags = self._parse_source_flags(source)
5755

5856
def _parse_source_files(self, source):
59-
files = []
60-
files.append(self._get_source_path())
61-
return files
57+
return [self._get_source_path()]
6258

6359
def _expected_fail(self):
6460
path = self.path
@@ -83,9 +79,10 @@ def _get_source_path(self):
8379
# Try .js first, and fall back to .mjs.
8480
# TODO(v8:9406): clean this up by never separating the path from
8581
# the extension in the first place.
86-
if os.path.exists(self._base_path + self._get_suffix()):
87-
return self._base_path + self._get_suffix()
88-
return self._base_path + '.mjs'
82+
js_path = self.suite.root / self.path_js
83+
if js_path.exists():
84+
return js_path
85+
return self.suite.root / self.path_mjs
8986

9087
def skip_predictable(self):
9188
# Message tests expected to fail don't print allocation output for
@@ -95,7 +92,7 @@ def skip_predictable(self):
9592
@property
9693
def output_proc(self):
9794
return message.OutProc(self.expected_outcomes,
98-
self._base_path,
95+
self.suite.root / self.path,
9996
self._expected_fail(),
100-
self._base_path + '.out',
97+
self.suite.root / self.path_and_suffix('.out'),
10198
self.test_config.regenerate_expected_files)

0 commit comments

Comments
 (0)