google / truth

Fluent assertions for Java and Android
https://truth.dev/
Apache License 2.0
2.72k stars 259 forks source link

A context-aware TestVerb for contextual assertions #323

Open dimo414 opened 7 years ago

dimo414 commented 7 years ago

Off-the-wall idea, posting here to see what others think:

I'm envisioning using Truth to verify some data processing involving nested structures (think traversing a tree and asserting things about individual nodes). The API might look something like:

try (ContextualTruth contextualAssert =
    Truth.withContext("Tree is in state %s", tree.getState()) {
  // this, on the other hand, will include the "Tree is in state"
  // message in the error if the assertion fails
  withContextAssert.that(bar).isEqualTo(expectedBar);
}

Once the try-with-resource block ends, the context is removed. Conversely it'd be possible to nest further .withContext() calls in order to add additional context.

I'm imagining this would be implemented by decorated FailureStrategy instances, managed by the ContextualTruth instances.


One issue I have with this idea (which makes me lean somewhat away from doing it) is it would make chained assertions more compelling, since every assertion now has the potential to implicitly have additional context. In the abstract I like the idea of this sort of context-injection, but I'm not certain it's worth the costs in terms of complexity. Curious what others think.

cpovirk commented 7 years ago

Relevant: #266

Also relevant: @PeteGillinGoogle's (currently Google-internal) proposal to attach arbitrary context to an individual assertion. If we were to do that, we could allow (if we think it's advisable :)):

TestVerb contextualAssert = assertWithContext("Tree is in state %s", tree.getState());
contextualAssert.that(bar).isEqualTo(expectedBar);

That's nice for repeating the same context for several consecutive statements, but it's not nice for accumulating context gradually across methods (or maybe it is, if you pass the TestVerb around, and maybe it's even preferable to be explicit? Are you envisioning a ThreadLocal?).

(I may well continue to be slow to respond here, as you're sadly used to by now.)

dimo414 commented 7 years ago

I don't have a fully-baked implementation in mind, but I think you could just compose immutable FailureStrategy instances into a stack, no need for ThreadLocal or any mutable state.

And no need for timeliness here, I've no immediate plans to implement anything. Mostly interested in soliciting thoughts at this point.

PeteGillin commented 7 years ago

For what it's worth, I am currently only +0.5 on my own proposal. The more I looked into it, the more API surface I discovered that it would have to touch (look at the amount of API surface touched by "withMessage" et al, it's basically the same), and the less it seemed worthwhile.

At the same time, there was a side-proposal that occurred to me and I think independently to Chris that you would often want to make a whole group of assertions in the same context. At that point, something more like @dimo414's proposal seems very sensible. That would minimize the API surface, I think. (I had at one point sketched out something like that, but I don't think I ever got anywhere with it, I can't remember.)

A related thing is that if you are grouping assertions in a context like this, you might want something that I think of as "scoped Expect", i.e. failures are not thrown immediately but stored and thrown at the end of the scope (c.f. the existing Expect where they're thrown on tearDown). If you're asserting that an object has three properties, you might well want to know if multiple of those properties are not satisfied. I think I discussed this with @cpovirk (but I can't remember where? sorry, memory not good today).

I should point out that, while I think this is all interesting stuff, none of it is close to the top of my priority list right now.

cpovirk commented 7 years ago

I think I found the internal conversation (on Pete's aforementioned assertInContext proposal), and I've CCed Pete and Michael.

(I agree with the overall perspective that this is all interesting and worth considering yet unlikely to be implemented anytime soon. Still, it's good to get the ideas out there so that they can shape what we're doing currently -- e.g., so that we don't remove features that would have turned out to be useful in combination with other future features or so that we can identify cases in which we can combine multiple independent ideas into a unified feature.)

dimo414 commented 7 years ago

Now that FailureMetadata is a thing, does this proposal make any more or less sense?

PeteGillin commented 7 years ago

More sense, I think???

cpovirk commented 7 years ago

Agreed. I'm still not sure exactly what I'm going to try to do this quarter with failure messages, but I should at minimum keep this in mind so that it is possible to add later.