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

eq matcher with temporary values #9

Open
almondtools opened this issue Apr 28, 2019 · 5 comments
Open

eq matcher with temporary values #9

almondtools opened this issue Apr 28, 2019 · 5 comments

Comments

@almondtools
Copy link

almondtools commented Apr 28, 2019

I am not certain how to succintly express an assert in following context (shortened, abstract example):

Assume following context:

#[derive(PartialEq, Debug)]
struct S {
  val:u32
}
impl S {
  pub fn new(val:u32) -> Self {
    S {val:val}
  }
}

The test (1):

fn test() {
  let s = S::new(42);
  
  assert_that!(s, eq(S::new(42)));
  assert_that!(s, not(eq(S::new(41))));
}

will not compile because s is consumed in the first assert statement.

The test (2):

fn test() {
  let s = S::new(42);
  
  assert_that!(&s, eq(S::new(42)));
  assert_that!(&s, not(eq(S::new(41))));
}

will not compile because the eq-matcher does not support unwrapping references.

The test (3):

fn test() {
  let s = S::new(42);
  
  assert_that!(&s, eq(&S::new(42)));
  assert_that!(&s, not(eq(&S::new(41))));
}

will not compile with 'temporary value dropped while borrowed'.

I could fix test (3) by introducing temporary variables, yet the test gets quite ugly with temporary variables.

My questions:
Is there another solution to test what I expect?
Is it planned to support something similar to test (2)?

@Valloric
Copy link
Owner

Why not just do this?

fn test() {
  assert_that!(&S::new(42), eq(&S::new(42)));
  assert_that!(&S::new(42), not(eq(&S::new(41))));
}

Another approach:

  #[derive(PartialEq, Debug)]
  #[cfg_attr(test, derive(Clone))]  // Notice we derive Clone in test builds
  struct S {
    val: u32
  }

  impl S {
    pub fn new(val: u32) -> Self {
      S { val }
    }
  }

  #[test]
  fn test() {
    let s = S::new(42);

    assert_that!(s.clone(), eq(S::new(42)));
    assert_that!(s.clone(), not(eq(S::new(41))));
  }

We did have support for auto-unwrapping references, but it had to be reverted because the way it was implemented broke backwards compatibility. Still looking for a way to implement that (PRs welcome, I'm out of ideas :)).

@almondtools
Copy link
Author

The first solution id a variant of (3) and fails because of a dropped temporary value. You probably meant:

fn test() {
  assert_that!(S::new(42), eq(S::new(42)));
  assert_that!(S::new(42), not(eq(S::new(41))));
}

which works, yet it is harder to read (from my point of view).

Working and more concise (but also misleading in my opinion):

fn test() {
  let s = || S::new(42);
  assert_that!(s(), eq(S::new(42)));
  assert_that!(s(), not(eq(S::new(41))));
}

Using clone() works, yet reading such test code would make me feel that the clone() method is tested rather than the new(...) method.

I had several ideas, maybe you have tried some of them:

  • I did not (yet) manage to let EqualTo have multiple matches(&self, actual) methods (maybe I am to new to Rust, as I understand you it is not easy for you either).
  • Next idea: Extend the assert_that! macro (I am not familiar with macros yet)
    • by changing the macro implementation
    • by inventing a new macro assert_that_ref!
  • Another idea: Provide a new matcher eq_ref for references

@Valloric
Copy link
Owner

I think the variant with calls to clone is fine and pretty readable. Anyone familiar with Rust will be familiar with what .clone() does so I don't expect it would surprise folks. Your approach with a lambda is also fine.

Provide a new matcher eq_ref for references

That's my current train of thought as well.

@almondtools
Copy link
Author

Yet my idea will conflict with backward compatibility, however my thoughts about this:

What is more intuitive when asserting a statement on some argument:

  1. the assertion consumes the argument (such that it is not usable any more)
  2. the assertion borrows the argument (the argument should be dropped at the end of its lifetime)

I would vote for 2. Talking about an object does not affect its state is more like borrowing than consuming. Maybe an idea for the next major version?

@strottos
Copy link

I would strongly support this, I realise this is an old issue request now and maybe this repo isn't being maintained. The workarounds suggested don't always work, if something isn't cloneable for example and taking references often gives the temporary value dropped while borrowed which isn't obvious how to workaround other than cloning.

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