[hermes] Add UI testing #3003
[hermes] Add UI testing #3003
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
joshlf
force-pushed
the
G1bd8ca80c7b97b4c799cec1504d281ae79f329b1
branch
from
cd76106 to
cdbf265
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## G3b521f32cf769ffedd72f84e60f1e73a8d590e4c #3003 +/- ##
==========================================================================
Coverage 91.87% 91.87%
==========================================================================
Files 20 20
Lines 6057 6057
==========================================================================
Hits 5565 5565
Misses 492 492 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a UI testing framework for the hermes tool, which is a valuable addition for ensuring correctness and providing clear error reporting. The implementation uses the ui_test crate and a shim to make hermes behave like rustc for testing purposes. My review has identified one critical issue in the test setup that would prevent the tests from running, along with several medium-severity suggestions to improve the robustness and error handling of the test shim itself. Overall, this is a solid foundation for UI testing.
| assert!(status.success(), "Failed to build binary '{}'", name); | ||
|
|
||
| let mut path = PathBuf::from(manifest_dir); | ||
| path.push(".."); |
There was a problem hiding this comment.
The path to the compiled binary is constructed incorrectly. The target directory is located at the workspace root, which is two levels above the hermes crate's manifest directory (tools/hermes/). The current code only goes up one level (..), which will result in an incorrect path (tools/target/...). This will cause the assert! on line 38 to fail. You need to go up two levels to find the workspace's target directory.
| path.push(".."); | |
| path.push(".."); | |
| path.push(".."); |
| }); | ||
|
|
||
| // Run logic with JSON emitter | ||
| let source = fs::read_to_string(&file_path).unwrap_or_default(); |
There was a problem hiding this comment.
Using unwrap_or_default() can hide file reading errors. If fs::read_to_string fails, it will proceed with an empty string, and the test will likely pass silently because no hermes items will be found. This can make debugging tests more difficult. It's better to panic with a clear message if the test file cannot be read.
| let source = fs::read_to_string(&file_path).unwrap_or_default(); | |
| let source = fs::read_to_string(&file_path).expect("Failed to read test file"); |
| parse::visit_hermes_items_in_file(&file_path, &source, |res| { | ||
| if let Err(e) = res { | ||
| has_errors = true; | ||
| emit_rustc_json(&e, &source, file_path.to_str().unwrap()); |
There was a problem hiding this comment.
Calling .unwrap() on file_path.to_str() can cause a panic if the file path contains non-UTF-8 characters, which is possible on some systems. It's safer and more idiomatic to use display() to get a string representation of the path, which gracefully handles non-UTF-8 characters.
| emit_rustc_json(&e, &source, file_path.to_str().unwrap()); | |
| emit_rustc_json(&e, &source, &file_path.display().to_string()); |
| byte_start: offset, | ||
| byte_end: offset + len, | ||
| line_start, | ||
| line_end: line_start, // Assuming single line for simplicity |
There was a problem hiding this comment.
The current implementation assumes that errors span only a single line by setting line_end: line_start. While this is sufficient for the current error types, it could produce incorrect diagnostic spans for more complex, multi-line errors in the future. To make this more robust, consider calculating line_end properly based on the full span of the error.
| rendered: format!("error: {}\n", msg), | ||
| }; | ||
|
|
||
| eprintln!("{}", serde_json::to_string(&diag).unwrap()); |
There was a problem hiding this comment.
Using .unwrap() can cause a panic without a descriptive message if JSON serialization fails. While unlikely for this structure, it's good practice to use .expect() to provide a more informative error message in case of a panic, which aids in debugging.
| eprintln!("{}", serde_json::to_string(&diag).unwrap()); | |
| eprintln!("{}", serde_json::to_string(&diag).expect("Failed to serialize diagnostic to JSON")); |
gherrit-pr-id: G1bd8ca80c7b97b4c799cec1504d281ae79f329b1
joshlf
force-pushed
the
G1bd8ca80c7b97b4c799cec1504d281ae79f329b1
branch
from
cdbf265 to
a113bca
Compare
joshlf
force-pushed
the
G3b521f32cf769ffedd72f84e60f1e73a8d590e4c
branch
from
298708d to
cd1ecc6
Compare
2 participants
mod foo;declarations #3008Latest Update: v35 — Compare vs v34
📚 Full Patch History
Links show the diff between the row version and the column version.
⬇️ Download this PR
Branch
git fetch origin refs/heads/G1bd8ca80c7b97b4c799cec1504d281ae79f329b1 && git checkout -b pr-G1bd8ca80c7b97b4c799cec1504d281ae79f329b1 FETCH_HEADCheckout
git fetch origin refs/heads/G1bd8ca80c7b97b4c799cec1504d281ae79f329b1 && git checkout FETCH_HEADCherry Pick
git fetch origin refs/heads/G1bd8ca80c7b97b4c799cec1504d281ae79f329b1 && git cherry-pick FETCH_HEADPull
Stacked PRs enabled by GHerrit.