alamb commented on code in PR #17911:
URL: https://github.com/apache/datafusion/pull/17911#discussion_r2407642644


##########
datafusion/proto/tests/cases/roundtrip_logical_plan.rs:
##########
@@ -2659,16 +2660,24 @@ async fn roundtrip_custom_listing_tables_schema() -> 
Result<()> {
         .infer_schema(&ctx.state())
         .await?;
 
-    ctx.register_table("hive_style", 
Arc::new(ListingTable::try_new(config)?))?;
+    let listing_table: Arc<dyn TableProvider> = 
Arc::new(ListingTable::try_new(config)?);
 
-    let plan = ctx

Review Comment:
   What I think it best is if the changes to the tests illustrate the changes 
in the code on the behavior of the code
   
   If you change the test **and** the code in the same PR it is harder to 
evaluate the impact of your change on behavior. 
   
   For example, if the original query now fails, perhaps you can change it to 
   ```rust
   let err = thing_that_errors.unwrap_err().to_string();
   assert_contains!(err, <expected message>)
   ```
   
   To show that a test that (incorrectly) used to work no longer does



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to