Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

When generating assert the right side should be the "correct" side. #125

Open
max-ishere opened this issue Jul 8, 2023 · 21 comments
Open

Comments

@max-ishere
Copy link

I have this testcase:

#[test_case(false => true; "Should invert")]
fn foo(statement: bool) -> bool {
    statement
}

And the error I get is this:

left: `true`,
right: `false`,

It would be nice if when I read "right" it would be the corrent (right) expression.

@AugustoFKL
Copy link
Contributor

@max-ishere, is there any reason for this besides it being "nice"?

The left item being the expected one is is the conventional ordering used in many popular testing frameworks such as JUnit, NUnit, and Python's unittest. In this convention, the expected value is placed first, followed by the actual value.

@ethanmsl
Copy link

ethanmsl commented Dec 27, 2023

@AugustoFKL

I can't find formal note of this as style, but it is the style of basically all rust examples.

assert!(result, expected)

see, for example here in Rust std docs

//Doc Test

let a = [1, 2, 3];
assert_eq!(a.iter().count(), 3);

let a = [1, 2, 3, 4, 5];
assert_eq!(a.iter().count(), 5);

I agree with max, it's very jarring to have <result, expected> everywhere, and then reversed specifically in test_cases where an output is specified. I know I have to spend enough mental cycles that I use the library much less than i otherwise would.

(And it's not helped by the fact that the macros is literally #[test_case( input, input => expected)], which also, visually, matches the 'calculated, expected' layout.)

So yeah: strong +1 for switching the order.

left = calculated
right = expected

@AugustoFKL
Copy link
Contributor

@ethanmsl, the problem is that the same reference you sent me uses both:

let a = [1, 2, 3];

let mut iter = a.iter();

// A call to next() returns the next value...
assert_eq!(Some(&1), iter.next());
assert_eq!(Some(&2), iter.next());
assert_eq!(Some(&3), iter.next());

// ... and then None once it's over.
assert_eq!(None, iter.next());

// More calls may or may not return `None`. Here, they always will.
assert_eq!(None, iter.next());
assert_eq!(None, iter.next());

But I see where @max-ishere is coming from. Indeed, the order that we use is left as input and right as expectancy, inverting it seems confusing, considering the absense of a set-to-stone rule regarding assertion positioning in Rust.

Therefore, I agree that in this scenario it would be more reasonable to invert them. I can work on having a PR for this in the next 2-3 weeks.

@ethanmsl
Copy link

Yes, I saw that after posting, and, just looking now found a couple more.
These are very much int he minority style, but it prompted me to search a bit more for the rust documentation style guide and ... there's no mention either way.

I've reached out to the #docs channel to see if they have any thoughts on norms or expectations.

So, there's no hard guideline I can point to. But my own background eyeball stats (not 100% reliable) suggest that
( calculated, expected ) is very much the style norm and what people will tend to expect in rust.

My personal defaults also just map input1, input2 => output to calculated => expected, so I think it's worth considering.
(also, if I'm hammering on a problem, the top or leftmost element is most significant and it will be the calculation that I'm looking at in these cases -- though granted that's a more minor point probably)

TLDR:
still a strong vote for expected on the right from me :) , but no formal standard for rust that I've yet found

AugustoFKL added a commit to AugustoFKL/test-case that referenced this issue Jan 3, 2024
… cases

Now the right side is the 'expected' one, and the left side is the 'input'. Example:

```rust
 #[test_case(2, 2 => 2 + 3)]
 fn result_which_panics(x: u32, y: u32) -> u32 {
     x + y
 }
```

Previously, the output was:

```
 Left:  5
 Right: 4
```

Now, it is:

```
 Left:  4
 Right: 5
```

Refs: frondeus#125
AugustoFKL added a commit to AugustoFKL/test-case that referenced this issue Jan 3, 2024
Now the right side is the 'expected' one, and the left side is the 'input'. Example:

```rust
 #[test_case(2, 2 => 2 + 3)]
 fn result_which_panics(x: u32, y: u32) -> u32 {
     x + y
 }
```

Previously, the output was:

```
 Left:  5
 Right: 4
```

Now, it is:

```
 Left:  4
 Right: 5
```

Refs: frondeus#125
AugustoFKL added a commit to AugustoFKL/test-case that referenced this issue Jan 3, 2024
Now the right side is the 'expected' one, and the left side is the 'input'. Example:

```rust
 #[test_case(2, 2 => 2 + 3)]
 fn result_which_panics(x: u32, y: u32) -> u32 {
     x + y
 }
```

Previously, the output was:

```
 Left:  5
 Right: 4
```

Now, it is:

```
 Left:  4
 Right: 5
```

Refs: frondeus#125
@AugustoFKL
Copy link
Contributor

@ethanmsl @max-ishere FYI, fixed this specific problem on the open PR #140

AugustoFKL added a commit to AugustoFKL/test-case that referenced this issue Jan 5, 2024
Now the right side is the 'expected' one, and the left side is the 'input'. Example:

```rust
 #[test_case(2, 2 => 2 + 3)]
 fn result_which_panics(x: u32, y: u32) -> u32 {
     x + y
 }
```

Previously, the output was:

```
 Left:  5
 Right: 4
```

Now, it is:

```
 Left:  4
 Right: 5
```

Refs: frondeus#125
AugustoFKL added a commit to AugustoFKL/test-case that referenced this issue Jan 5, 2024
Now the right side is the 'expected' one, and the left side is the 'input'. Example:

```rust
 #[test_case(2, 2 => 2 + 3)]
 fn result_which_panics(x: u32, y: u32) -> u32 {
     x + y
 }
```

Previously, the output was:

```
 Left:  5
 Right: 4
```

Now, it is:

```
 Left:  4
 Right: 5
```

Refs: frondeus#125
@AugustoFKL
Copy link
Contributor

@ethanmsl @frondeus I think we can close this, no?

It is already fixed by #140

@max-ishere
Copy link
Author

I tested this on 3.3.1 and the output is still

assertion `left == right` failed
  left: "expected"
 right: "calc"
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Was the change reverted?

@AugustoFKL
Copy link
Contributor

@max-ishere, the affected scenario was 'a => b'. 'a == b' still has the same behavior.

I'd love to add a patch for that, but sadly I'm quite overwhelmed at the moment.

@max-ishere
Copy link
Author

I don't understand... I haven't read the code but isn't it all the same macro? It should always have right as the correct side if this was actually implemented.

@AugustoFKL
Copy link
Contributor

@max-ishere, yes and no. The code internally has a match that handled the syntax for different structures.

a => b and a == b are handled in the same place, but different match arms.

@max-ishere
Copy link
Author

I don't understand. The => syntax is in the macro. That wasn't changing. The assert was. Was the assert not updated?

@AugustoFKL
Copy link
Contributor

Sorry @max-ishere, maybe I misunderstood. Can you give a real example of what you're testing, please? The scenario described in the issue description was resolved, but maybe you expected an underlying modification that I wasn't aware of.

@max-ishere
Copy link
Author

I tested this on 3.3.1 and the output is still

assertion `left == right` failed
  left: "expected"
 right: "calc"
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Was the change reverted?

This was the output of #[test_case("calc" => "expected")]. I expected that if the issue was resolved the output would be:

left: "calc"
right: "expected"

@max-ishere
Copy link
Author

As you can see LHS and RHS in assert_eq macro are flipped.

@AugustoFKL
Copy link
Contributor

@max-ishere

Actually, I just noticed that there wasn't a release with the PR I've made. Please reach out to @frondeus, as I'm not really aware of when the next release will be.

@max-ishere
Copy link
Author

Alright then. Since you already pinged them I won't. Just gonna wait for the release then

@AugustoFKL
Copy link
Contributor

@luke-biel, is there an estimate on the next release date?

@max-ishere
Copy link
Author

Can't we just release a patch with this change?

@AugustoFKL
Copy link
Contributor

@max-ishere that's why I marked @luke-biel. I'm no maintainer.

@max-ishere
Copy link
Author

It's been a year

@AugustoFKL
Copy link
Contributor

@max-ishere, yeah... It seems the maintainers are not available

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants