[test] Parameterize BytecodeGeneratorTest over golden files [v8/v8 : main]
0 views
Skip to first unread message
Leszek Swirski (Gerrit)
Sep 4, 2025, 5:46:19 AM (3 days ago) Sep 4
to Patrick Thier, V8 LUCI CQ, v8-re...@googlegroups.com
Attention needed from Patrick Thier
Leszek Swirski voted and added 1 comment![]( "Open in Gerrit")
Votes added by Leszek Swirski
| Auto-Submit | +1 |
| Commit-Queue | +1 |
1 comment
Patchset-level comments
File-level comment, Patchset 2 (Latest):
Leszek Swirski . resolved
PTAL, should be the last one before I leave these tests alone.
Related details
Attention is currently required from:
- Patrick Thier
Submit Requirements:
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Thier (Gerrit)
Sep 4, 2025, 6:48:48 AM (3 days ago) Sep 4
to Leszek Swirski, V8 LUCI CQ, v8-re...@googlegroups.com
Attention needed from Leszek Swirski
Patrick Thier voted and added 2 comments![]( "Open in Gerrit")
Votes added by Patrick Thier
| Code-Review | +1 |
2 comments
Patchset-level comments
Patrick Thier . resolved
Nice! LGTM
File test/unittests/interpreter/bytecode-expectations-parser.h
Line 40, Patchset 2 (Latest):
bool CollectGoldenFiles(std::vector<std::string>* golden_file_list,Patrick Thier . unresolved
nit: `const std::vector<std::string>&`
Related details
Attention is currently required from:
- Leszek Swirski
Submit Requirements:
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Sep 4, 2025, 7:33:04 AM (3 days ago) Sep 4
to Patrick Thier, V8 LUCI CQ, v8-re...@googlegroups.com
Leszek Swirski voted and added 1 comment![]( "Open in Gerrit")
Votes added by Leszek Swirski
| Auto-Submit | +1 |
| Commit-Queue | +2 |
1 comment
File test/unittests/interpreter/bytecode-expectations-parser.h
Line 40, Patchset 2:
bool CollectGoldenFiles(std::vector<std::string>* golden_file_list,Patrick Thier . resolved
nit: `const std::vector<std::string>&`
Leszek Swirski
Actually I can do one better and return a std::vector.
Related details
Attention set is empty
Submit Requirements:
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
V8 LUCI CQ (Gerrit)
Sep 4, 2025, 8:05:15 AM (3 days ago) Sep 4
to Leszek Swirski, Patrick Thier, v8-re...@googlegroups.com
V8 LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
2 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: test/unittests/interpreter/bytecode-expectations-parser.h
Insertions: 1, Deletions: 2.
@@ -37,8 +37,7 @@
int current_line_ = 0;
};
-bool CollectGoldenFiles(std::vector<std::string>* golden_file_list,
- const char* directory_path);
+std::vector<std::string> CollectGoldenFiles(const char* directory_path);
} // namespace v8::internal::interpreter
```
```
The name of the file: test/unittests/interpreter/bytecode-generator-unittest.cc
Insertions: 12, Deletions: 17.
@@ -158,23 +158,18 @@
}
}
-INSTANTIATE_TEST_SUITE_P(AllGolden, BytecodeGeneratorTest,
- // Run the BytecodeGeneratorTest for each golden file
- // in the golden file directory.
- testing::ValuesIn(([] {
- std::vector<std::string> golden_files;
- CHECK(CollectGoldenFiles(&golden_files,
- kGoldenFileDirectory));
- return golden_files;
- })()),
- // Print the golden file's filename, without the
- // extension, as the test value.
- [](const testing::TestParamInfo<std::string>& info) {
- std::string file = info.param;
- CHECK(file.ends_with(".golden"));
- return file.substr(
- 0, file.length() - strlen(".golden"));
- });
+INSTANTIATE_TEST_SUITE_P(
+ AllGolden, BytecodeGeneratorTest,
+ // Run the BytecodeGeneratorTest for each golden file
+ // in the golden file directory.
+ testing::ValuesIn(CollectGoldenFiles(kGoldenFileDirectory)),
+ // Print the golden file's filename, without the
+ // extension, as the test value.
+ [](const testing::TestParamInfo<std::string>& info) {
+ std::string file = info.param;
+ CHECK(file.ends_with(".golden"));
+ return file.substr(0, file.length() - strlen(".golden"));
+ });
} // namespace interpreter
} // namespace internal
```
```
The name of the file: test/unittests/interpreter/generate-bytecode-expectations.cc
Insertions: 2, Deletions: 4.
@@ -147,9 +147,7 @@
if (options.verbose_) {
std::cout << "Looking for golden files in " << kGoldenFilesPath << '\n';
}
- if (!CollectGoldenFiles(&options.input_filenames_, kGoldenFilesPath)) {
- FATAL("Golden files autodiscovery failed.");
- }
+ options.input_filenames_ = CollectGoldenFiles(kGoldenFilesPath);
}
return options;
@@ -369,7 +367,7 @@
std::string actual =
WriteExpectationsToString(snippet_list, platform, options);
- std::ifstream input_stream(input_filename);
+ std::ifstream input_stream(kGoldenFilesPath + input_filename);
if (!input_stream.is_open()) {
FATAL("Could not open %s for reading.", input_filename.c_str());
}
```
```
The name of the file: test/unittests/interpreter/bytecode-expectations-parser.cc
Insertions: 7, Deletions: 6.
@@ -37,7 +37,7 @@
bool previous_was_backslash = false;
for (char c : escaped_string) {
if (previous_was_backslash) {
- // If it was not an escape sequence, emit the previous backslash
+ // If it was not an escape sequence, emit the previous backslash.
if (c != '\\' && c != '"') unescaped_string += '\\';
unescaped_string += c;
previous_was_backslash = false;
@@ -50,6 +50,10 @@
}
}
}
+ if (previous_was_backslash) {
+ // Emit the previous backslash if it wasn't emitted.
+ unescaped_string += '\\';
+ }
return unescaped_string;
}
@@ -64,7 +68,7 @@
BytecodeExpectationsHeaderOptions options;
std::string line;
- // Skip to the beginning of the options header
+ // Skip to the beginning of the options header.
while (GetLine(line)) {
if (line == "---") break;
}
@@ -108,10 +112,10 @@
if (!found_begin_snippet) continue;
if (line == "\"") return true;
if (line.size() == 0) {
- string_out->append("\n"); // consume empty line
+ string_out->append("\n"); // consume empty line.
continue;
}
- CHECK_GE(line.size(), 2u); // We should have the indent
+ CHECK_GE(line.size(), 2u); // We should have the indent.
line = UnescapeString(line);
string_out->append(line.begin() + 2, line.end());
*string_out += '\n';
@@ -129,18 +133,19 @@
return out;
}
-bool CollectGoldenFiles(std::vector<std::string>* golden_file_list,
- const char* directory_path) {
- if (!std::filesystem::exists(directory_path)) return false;
- if (!std::filesystem::is_directory(directory_path)) return false;
+std::vector<std::string> CollectGoldenFiles(const char* directory_path) {
+ CHECK(std::filesystem::exists(directory_path));
+ CHECK(std::filesystem::is_directory(directory_path));
+
+ std::vector<std::string> ret;
for (const auto& entry :
std::filesystem::directory_iterator(directory_path)) {
if (!entry.is_regular_file()) continue;
if (entry.path().extension() == ".golden") {
- golden_file_list->push_back(entry.path().filename().string());
+ ret.push_back(entry.path().filename().string());
}
}
- return true;
+ return ret;
}
} // namespace v8::internal::interpreter
```
Change information
Commit message:
[test] Parameterize BytecodeGeneratorTest over golden files
This refactors the BytecodeGeneratorTest to be a single parameterized
test that runs on all `.golden` files in the bytecode expectations
directory.
This avoids the need to add a new `TEST_F` for each new golden file.
Test failures now also report the filename and line number of the
mismatch, making debugging easier.
The golden file discovery is now implemented using `<filesystem>`,
simplifying the code in both the unittest and the
`generate-bytecode-expectations` tool.
Change-Id: I689042941fcbeeb1ae8bf19fe8f44e757f0e5e60
Reviewed-by: Patrick Thier <pth...@chromium.org>
Commit-Queue: Leszek Swirski <les...@chromium.org>
Auto-Submit: Leszek Swirski <les...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#102244}
Files:
- M test/unittests/interpreter/bytecode-expectations-parser.cc
- M test/unittests/interpreter/bytecode-expectations-parser.h
- M test/unittests/interpreter/bytecode-expectations-printer.cc
- M test/unittests/interpreter/bytecode-generator-unittest.cc
- M test/unittests/interpreter/generate-bytecode-expectations.cc
Change size: L
Delta: 5 files changed, 101 insertions(+), 785 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Patrick Thier
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Achenbach (Gerrit)
Sep 5, 2025, 3:18:40 AM (2 days ago) Sep 5
to Leszek Swirski, V8 LUCI CQ, Patrick Thier, v8-re...@googlegroups.com
Michael Achenbach added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 4 (Latest):
Michael Achenbach . resolved
This breaks building fuzztest: https://ci.chromium.org/ui/p/v8/builders/ci/V8%20Centipede%20Linux64%20ASAN%20%20-%20release%20builder/10326/overview
There we run the unit test exe to list all existing fuzz tests, which now runs into some missing directory.
Related details
Attention set is empty
Submit Requirements:
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Sep 5, 2025, 3:36:16 AM (2 days ago) Sep 5
to V8 LUCI CQ, Michael Achenbach, Patrick Thier, v8-re...@googlegroups.com
Leszek Swirski added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Michael Achenbach . resolved
This breaks building fuzztest: https://ci.chromium.org/ui/p/v8/builders/ci/V8%20Centipede%20Linux64%20ASAN%20%20-%20release%20builder/10326/overview
There we run the unit test exe to list all existing fuzz tests, which now runs into some missing directory.
Leszek Swirski
Interesting, I'll take a look. Does this run in a full V8 checkout?
Related details
Attention set is empty
Submit Requirements:
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Achenbach (Gerrit)
Sep 5, 2025, 4:43:23 AM (2 days ago) Sep 5
to Leszek Swirski, V8 LUCI CQ, Patrick Thier, v8-re...@googlegroups.com
Michael Achenbach added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Michael Achenbach . resolved
Leszek SwirskiThis breaks building fuzztest: https://ci.chromium.org/ui/p/v8/builders/ci/V8%20Centipede%20Linux64%20ASAN%20%20-%20release%20builder/10326/overview
There we run the unit test exe to list all existing fuzz tests, which now runs into some missing directory.
Interesting, I'll take a look. Does this run in a full V8 checkout?
Michael Achenbach
Yes. Though not sure what alternatives you had in mind. The builder runs compilation as all our other builders. But it calls that exe as part of one build step.
Related details
Attention set is empty
Submit Requirements:
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages