apache / datafusion

Apache DataFusion SQL Query Engine
https://datafusion.apache.org/
Apache License 2.0
5.87k stars 1.11k forks source link

Decide whether we want to standardize which `assert_eq!` parameter ("left" or "right") should be used for "expected" value #11270

Open findepi opened 2 months ago

findepi commented 2 months ago

assert_eq! names its parameters "left" and "right" and doesn't seem to designate which one is expected and which is actual. However, when a test is run from RustRover, the IDE is apparently opinionated and currently expects the "right" to be "the expected" (see screenshot below).

Let's decide whether we as the project want to care about this. Currently, the code varies: example 1, example 2.

image

Some RustRover references

findepi commented 2 months ago

cc @alamb @jonahgao

alamb commented 2 months ago

I think consistency is a good thing. I personally prefer assert(actual, expected) but would follow whatever convention is chosen

According to the internet there appears to be no consensus across rust https://users.rust-lang.org/t/assert-eq-expected-and-actual/20304 (perhaps the left and right is a compromise between different opinions)

If we want to standardize (which seems like a good thing to me) it seems like we would:

  1. Get (lazy) consensus
  2. Add a note to the contributors guide: https://datafusion.apache.org/contributor-guide/index.html
  3. Make as much of the code consistent as possible
alamb commented 2 months ago

(Thank you for raising this @findepi )

Omega359 commented 1 month ago

FYI, somewhat related I do think RustRover has a bug around diffing results where it flips the expected and actual in the diff. See https://youtrack.jetbrains.com/issue/IJPL-157945. I need to provide a minimal test case for that

findepi commented 1 month ago

I agree @Omega359 this looks more like RustRover issue. It seems that RR has some limitations under which they are working (https://github.com/intellij-rust/intellij-rust/pull/3848). At the same time, confusion about what's expected and what's actual isn't helping anyone (but this only matters when a test fails).

I came across an assertions library for Rust: https://github.com/google/assertor. I like single easy entry point they have (assert_that!), with code completion for the assertions themselves. They also attempt to provide useful err messages. For example assert_that!(vec![2, 4, 6]).contains_exactly(vec![2, 5, 6]); prints

assertion failed: datafusion/core/src/myfile.rs:88:9
missing (1)   : [5]
unexpected (1): [4]

Does anyone have experiences with it in a larger code base?

Omega359 commented 1 month ago

That seems similar to the assertJ Java assertion library which I've used in the past. Fluent api's like that can be more verbose but in general I find them a lot easier to use.