[hermes] Add fixture testing #3010
Open
+102,011
−8
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Feb 7, 2026
Contributor
Summary of ChangesHello @joshlf, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the testing capabilities of the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
joshlf
force-pushed
the
G3220747e90b678c598c506be49219a8f75118988
branch
from
5940683 to
e477af0
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a robust fixture-based testing framework for hermes using datatest-stable and assert_cmd, which is a significant step towards ensuring the tool's correctness and stability. The refactoring of the command-line interface in main.rs to use subcommands is also a welcome change for future extensibility. I have a couple of suggestions to improve the test harness implementation regarding efficiency and robustness.
| let expected_content = fs::read_to_string(expected_path)?; | ||
| let actual_content = fs::read_to_string(&actual_path)?; |
Contributor
There was a problem hiding this comment.
Using fs::read_to_string will cause a panic if any of the test fixtures are binary files. To make this more robust, you should use fs::read to get the contents as bytes. Then, you can use String::from_utf8_lossy to handle both text and binary files gracefully before normalizing line endings.
let expected_bytes = fs::read(expected_path)?;
let actual_bytes = fs::read(&actual_path)?;
Comment on lines
+46
to
+80
| // Tests can specify the expected exit status. | ||
| let expected_status_file = test_case_root.join("expected_status.txt"); | ||
| let assert = if expected_status_file.exists() { | ||
| let status = fs::read_to_string(&expected_status_file)?; | ||
| if status.trim() == "failure" { | ||
| assert.failure() | ||
| } else { | ||
| assert.success() | ||
| } | ||
| } else { | ||
| // Default to expecting success | ||
| assert.success() | ||
| }; | ||
|
|
||
| // Tests can specify the expected stderr. | ||
| let expected_stderr_file = test_case_root.join("expected_stderr.txt"); | ||
| if expected_stderr_file.exists() { | ||
| let needle = fs::read_to_string(expected_stderr_file)?; | ||
| assert.stderr(predicates::str::contains(needle.trim())); | ||
| } | ||
|
|
||
| // Tests can specify the expected shadow crate content. | ||
| let actual_shadow = sandbox_root.join("target/hermes_shadow"); | ||
| let expected_shadow = test_case_root.join("expected"); | ||
|
|
||
| if expected_shadow.exists() { | ||
| assert_directories_match(&expected_shadow, &actual_shadow)?; | ||
| } else { | ||
| // If the test expects failure, we may not have a shadow crate to check. | ||
| // Only warn if we expected success but found no 'expected' dir. | ||
| if !expected_status_file.exists() | ||
| || fs::read_to_string(&expected_status_file)?.trim() != "failure" | ||
| { | ||
| eprintln!("WARNING: No 'expected' directory found for test case '{}'", test_name); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
This block reads expected_status.txt twice: once to set the assertion and once to decide whether to print a warning. This is inefficient. You can refactor this logic to read the file only once, store its content, and reuse it.
// Tests can specify the expected exit status.
let expected_status_file = test_case_root.join("expected_status.txt");
let expects_failure = if let Ok(status) = fs::read_to_string(&expected_status_file) {
status.trim() == "failure"
} else {
false
};
let assert = if expects_failure {
assert.failure()
} else {
assert.success()
};
// Tests can specify the expected stderr.
let expected_stderr_file = test_case_root.join("expected_stderr.txt");
if expected_stderr_file.exists() {
let needle = fs::read_to_string(expected_stderr_file)?;
assert.stderr(predicates::str::contains(needle.trim()));
}
// Tests can specify the expected shadow crate content.
let actual_shadow = sandbox_root.join("target/hermes_shadow");
let expected_shadow = test_case_root.join("expected");
if expected_shadow.exists() {
assert_directories_match(&expected_shadow, &actual_shadow)?;
} else {
// If the test expects failure, we may not have a shadow crate to check.
// Only warn if we expected success but found no 'expected' dir.
if !expects_failure {
eprintln!("WARNING: No 'expected' directory found for test case '{}'", test_name);
}
}
joshlf
force-pushed
the
G3220747e90b678c598c506be49219a8f75118988
branch
from
e477af0 to
ec619c0
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## G2f234da34d5c9cd516c08526882baaee631fe7d2 #3010 +/- ##
============================================================================
Coverage ? 91.87%
============================================================================
Files ? 20
Lines ? 6057
Branches ? 0
============================================================================
Hits ? 5565
Misses ? 492
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
joshlf
force-pushed
the
G3220747e90b678c598c506be49219a8f75118988
branch
from
ec619c0 to
75e4b1c
Compare
Member
Author
|
/gemini review |
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a fixture-based integration testing framework for hermes using datatest-stable and assert_cmd. A comprehensive set of test cases has been added to cover various scenarios, including different module structures, error conditions, and workspace setups. The CLI has also been refactored to use subcommands, improving its extensibility. The changes are well-structured and significantly improve the project's test coverage. My main feedback is a suggestion to make the test argument parsing more robust.
| let args_file = test_case_root.join("args.txt"); | ||
| if args_file.exists() { | ||
| let args_str = fs::read_to_string(args_file)?; | ||
| cmd.args(args_str.trim().split_whitespace()); |
Contributor
There was a problem hiding this comment.
The use of split_whitespace() to parse arguments from args.txt is not robust for arguments that may contain spaces, such as file paths. This could lead to unexpected behavior where an argument is split into multiple parts.
For more robust argument parsing that handles quotes and spaces correctly, consider using the shlex crate. This will make the test harness more resilient to complex arguments in the future.
Example with shlex:
// In Cargo.toml, under [dev-dependencies]
// shlex = "1.1.0"
// In integration.rs
if args_file.exists() {
let args_str = fs::read_to_string(args_file)?;
match shlex::split(&args_str) {
Some(args) => cmd.args(args),
None => panic!("invalid shell-like syntax in args.txt: {}", args_str),
};
} else {
cmd.arg("verify");
}
This was referenced Feb 7, 2026
joshlf
force-pushed
the
G2f234da34d5c9cd516c08526882baaee631fe7d2
branch
from
6f377df to
4ae812e
Compare
joshlf
force-pushed
the
G3220747e90b678c598c506be49219a8f75118988
branch
from
75e4b1c to
03142cc
Compare
This was referenced Feb 10, 2026
gherrit-pr-id: G3220747e90b678c598c506be49219a8f75118988
joshlf
force-pushed
the
G3220747e90b678c598c506be49219a8f75118988
branch
from
03142cc to
db80a43
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
mod foo;declarations #3008Latest Update: v31 — Compare vs v30
📚 Full Patch History
Links show the diff between the row version and the column version.
⬇️ Download this PR
Branch
git fetch origin refs/heads/G3220747e90b678c598c506be49219a8f75118988 && git checkout -b pr-G3220747e90b678c598c506be49219a8f75118988 FETCH_HEADCheckout
git fetch origin refs/heads/G3220747e90b678c598c506be49219a8f75118988 && git checkout FETCH_HEADCherry Pick
git fetch origin refs/heads/G3220747e90b678c598c506be49219a8f75118988 && git cherry-pick FETCH_HEADPull
Stacked PRs enabled by GHerrit.