mustafasrepo commented on PR #6234: URL: https://github.com/apache/arrow-datafusion/pull/6234#issuecomment-1542157223
> > Other than these this PR is LGTM!. > > Thank you for the review @mustafasrepo -- I will make the changes > > > However, I wonder whether we should keep an example test case each file. Because, tests in .rs files are quite useful during development. > > Can you explain how SQL tests in .rs files are more useful during development than ones in .slt files? I suspect you are using a different workflow from me (which is totally good and understandable, but I therefore lack context). > > When I am working on some new feature, I found it quite easy to make some `new_feature.slt` file with just the SQL I want to run and then test with `cargo test --test sqllogictests -- new_feature`. This is often better for me than .rs tests because I can change the test and re-run it immediately without having to wait for it to recompile. > > > Novices can use these example test cases as starting point when they work on new functionality. I am not really sure, whether it will be helpful. What are you thoughts about it? > > My thoughts on this topic are: > > 1. I agree that for people starting out with DataFusion development .rs tests are easier (because they are familiar to all Rust developers). sqllogictests are harder because they require learning another tool (though I think the barrier is pretty low) > 2. However, for SQL based tests, sqllogictest based tests are _much_ easier to maintain (specifically updating the tests with `cargo test --test sqllogictests -- --complete`). In my opinion this long term benefit is worth the short term cost > > The reason I am spending effort porting (and asking others to help) port the existing tests to sqllogictest is precisely to discourage new SQL tests written in rust (so the code is easier to maintain long term) > > 🤔 if this makes sense to you, perhaps I can try to incorporate some of this rational into the documentation to make it clearer. My motivation was mainly 1. However, there are lots of example sqllogictest, one can see the pattern easily. Also it is not clear that single example test case in `.rs` file shouldn't grow, and examples should in the end go to `.slt` files. It should be communicated in every PR. In this case, I think it is better to have all sql tests in `.slt` files. Thanks for clearification. -- 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]
