Conversation
Signed-off-by: discord9 <discord9@163.com>
Summary of ChangesHello @discord9, 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 introduces a new "skip marker" feature to the Highlights
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
|
![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 SkipInterceptor to allow skipping test cases. The implementation includes the interceptor logic, integration into the runner, and logic to handle skipped tests during result comparison. While the feature is useful, I've found two critical issues. First, the SkipInterceptor itself is flawed and won't produce any output due to how the test case execution is structured. Second, the logic in the runner for handling result files of skipped tests can lead to misleading results, potentially hiding the skipped status of a test. I've provided detailed comments and suggestions for how to fix these issues.
| impl Interceptor for SkipInterceptor { | ||
| fn before_execute(&self, execute_query: &mut Vec<String>, _context: &mut crate::QueryContext) { | ||
| execute_query.clear(); | ||
| } | ||
|
|
||
| fn after_execute(&self, result: &mut String) { | ||
| *result = format!("{} {}", SKIP_MARKER_PREFIX, self.reason); | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation of SkipInterceptor is unfortunately flawed due to the execution flow in TestCase::execute (in sqlness/src/case.rs).
By calling execute_query.clear() in before_execute, you prevent any SQL from being run. However, this also means the loop over queries in TestCase::execute is skipped, and consequently, after_execute_intercept (which calls after_execute) is never called. As a result, the skip marker is never written to the result file.
The unit test for this interceptor doesn't catch this because it calls before_execute and after_execute in isolation, not through the TestCase::execute flow.
To fix this, the logic in TestCase::execute needs to be adjusted to handle cases where the query becomes empty after before_execute_intercept. For example, it could call after_execute_intercept even if there's no SQL to run.
Here's a possible modification for sqlness/src/case.rs:
// in sqlness/src/case.rs
pub(crate) async fn execute<W>(&mut self, db: &dyn Database, writer: &mut W) -> Result<()>
where
W: Write,
{
let context = self.before_execute_intercept().await;
// ... (writing comments and display query)
let sql = self.concat_query_lines();
if sql.trim().is_empty() {
// If query is empty (e.g. by interceptor), we still need to run after_execute
let mut result = String::new();
self.after_execute_intercept(&mut result).await;
if !result.is_empty() {
self.write_result(writer, result)?;
}
} else {
for sql in sql.split(crate::interceptor::template::DELIMITER) {
if !sql.trim().is_empty() {
let sql = if sql.ends_with(QUERY_DELIMITER) {
sql.to_string()
} else {
format!("{sql};")
};
let mut result = db.query(context.clone(), sql).await.to_string();
self.after_execute_intercept(&mut result).await;
self.write_result(writer, result)?;
}
}
}
Ok(())
}Since case.rs is not in this PR's diff, I cannot provide a direct code suggestion for that file. However, the SkipInterceptor as it is will not work.
| // Compare old and new result | ||
| let new_result = String::from_utf8(new_result.into_inner()).expect("not utf8 string"); | ||
| let diff_only_skip_markers = self.are_all_changes_skip_markers_from_strings(&old_result, &new_result); | ||
|
|
||
| // If diff is only SKIP markers, write back old_result to keep result file clean | ||
| let result_to_write = if diff_only_skip_markers { | ||
| &old_result | ||
| } else { | ||
| &new_result | ||
| }; | ||
|
|
||
| // Truncate and write result back | ||
| result_file.set_len(0)?; | ||
| result_file.rewind()?; | ||
| result_file.write_all(new_result.get_ref())?; | ||
| result_file.write_all(result_to_write.as_bytes())?; |
There was a problem hiding this comment.
The current logic for writing the result file when a test is skipped can lead to misleading results. If a test case produced a different result previously (e.g., a failing result, or a correct result from a passing test), and is now skipped, the old result will be written back to the .result file, but the test will be marked as passed. This hides the fact that the test was skipped and can make it seem like the old result is still being produced.
The result file should always reflect the actual outcome of the test run. In the case of a skipped test, it should contain the skip marker. The compare function already correctly handles ignoring skip markers for determining pass/fail status.
I suggest always writing the new_result to the file, which simplifies the logic and ensures correctness.
let new_result = String::from_utf8(new_result.into_inner()).expect("not utf8 string");
// Truncate and write result back
result_file.set_len(0)?;
result_file.rewind()?;
result_file.write_all(new_result.as_bytes())?;1 participant
Rationale
Detailed Changes
Test Plan