Improve PropertyConstraint error output

I normally get far too distracted by interesting but niche things. This is one that bites me almost daily.

I use PropertyConstraint a lot - this is the error message that's currently shown when the property can't be found on the object.

I hit this regularly - either because my constraint is not actually on the object I think it is, because I go the property name wrong, or because I'm trying to look at an explicit interface implementation.

I want to improve this so it provides a proper error message rather than stack trace. What I'm interested in seeing is:

The property name I was looking for. (I almost always use nameof() here - maybe the string passed in wasn't what I thought)
The type of the object I'm looking for the property on.

I'll PR this at some point - any other suggestions before I do? I considered listing all available property names - but decided against it as it could get noisy quickly.

My guess is that a PropertyExistsConstraint probably gives a nicer message. That is...

Assert.That(myObject, Has.Property(nameof(SomeProperty)));

Does that seem to be the case?

It does - but testing for property existence isn't my end goal, I want to test property values.

I see these error messages while I'm writing my tests and making mistakes in the test code. I'm looking to instead show an error message which helps me debug what's wrong with the constraint I've written, rather than the raw stack trace we currently have. 🙂

I split the two constraints for reasons pertaining to the internal implementation. (The above syntax actually generates a constraint that resolves to either a PropertyExistsConstraint or a PropertyConstraint depending on what follows it.)

But I think an assumption crept in that you already knew the property exists when you create a PropertyConstraint. Of course you do - or think you do - but errors can happen. I think it would make sense if PropertyConstraint on a non-existent property would give the same sort of message as PropertyConstraint.

There's actually a TODO on PropertyConstraint!

// TODO: Use an error result here
if (property == null)
     throw new ArgumentException(string.Format("Property {0} was not found", name), "name");

I agree with the comment that a PropertyConstraint for a property that doesn't exist should be an error rather than a failure - but I'm more interested in putting a useful message with that. 🙂

But you already convinced me is should be a Failure! 😄

I can go either way though.

Sorry - I guess I haven't been clear!

I propose, that a PropertyConstraint looking for a property that does not exist, should have a ResultState of Error - but additionally a message which helps the user diagnose the issue, as opposed to the current raw stack trace.

I suggest it's an error rather than a failure, as the property doesn't exist - so the issue is developer error, rather than a failure of what the user was aiming to test. If the user wishes to test whether a property exists, they should be using the PropertyExistsConstraint.

My primary concern is seeing a more helpful message when I get it wrong - I'm less worried about how that message gets there, if other people have strong opinions. 🙂

Could we unify the implementations of PropertyExistsConstraint and PropertyConstraint?

Unifying the two constraints is possible. However, I split them to simplify the implementation under the covers, particularly with respect to the Property Operator and how it works within ConstraintBuilder. So that underlying stuff would have to change as well.

The only reason I ask is that it makes it easier to keep error messages and other behavior consistent.

We could have a common base class.

@ChrisMaddock

That sounds great! @ChrisMaddock do you feel like doing that? 😃

The main thing about errors, btw, is that you get a bigger stack trace in the report.

Hmm, I'm not terribly familiar with the difference between constraint errors and failures. When I was looking for inconclusive constraint result status I found this and assumed naturally that we don't use Error for anything:

https://github.com/nunit/nunit/blob/master/src/NUnitFramework/framework/Constraints/ConstraintResult.cs#L42-L50

In any case, the decision whether to fail or error can be abstracted in the base class.

@jnm2

@jnm2 Currently an error is signaled by the constraint throwing an exception. As a result, no ConstraintResult is ever produced.

I had the idea that we might want the constraint to catch exceptions and create it's own error report in the result. That would have allowed a Multiple Assert to continue in the face of an unexpected exception. In the end, I decided terminating on an unexpected exception was still the best course of action, so I expect we will never make use of that member.

@CharliePoole

@CharliePoole Ohh I'm following now! Yes. The old ArgumentException or InvalidOperationException("You're using me wrong.") that any class uses as part of its contract.

I have always thought that Has.Property("NonExistentName") and Has.Property("NonExistentName").EqualTo(x) should act exactly the same when the property does not exist. The constraint builder syntax strongly suggests this.

@ChrisMaddock

Well, @ChrisMaddock accidentally convinced me of that point of view in the first part of this thread, but it turned out I misunderstood him. 😃

My logic in implementing this was that Has.Property("Name") implies you don't know if the property exists and are testing for it, while Has.Property("Name").EqualTo("Something") means you are checking the value of a property, which you know is there. It seemed unlikely to me that you would be checking the value of a property, which might not exist. Clearly, it can be understood in other ways.

We should make sure that whatever we end up with is clearly stated in the docs.

I find it goes against the principle of least surprise that from the perspective of the syntax, the first constraint behaves differently when another constraint follows it. I'm not totally convinced of the value of the exception vs the failure result.

I had the idea that we might want the constraint to catch exceptions and create it's own error report in the result.

Oh - I thought this was a thing we could do - have a constraint report an error. That was my plan for this. 😞

I personally have no interest in whether my result is a failure or an error, but NUnit makes the distinction in it's result types, so I've tried to follow that in the sensible way. On that basis, it does make seem to make sense that PropertyExistsConstraint and Property(Value)Constraint return different errors/failures - but that doesn't mean the message reported to the user shouldn't be equally helpful in both cases.

I don't like when the same piece of syntax resolves to different actual constraint classes with differing behavior. It's counter-intuitive that Has.Property("foo").And.Property("foo").EqualTo(42) should act differently than Has.Property("foo").EqualTo(42).

@ChrisMaddock

@ChrisMaddock We're on the same page, I think. Throwing is how you "report" an error. The actual reporting is done upstream.

On errors versus failures, I think you probably rely on the distinction more than you think. Consider the example we are discussing and the following code...

Assert.That(actual, Has.Property("XYZ").EqualTo(123));
Assert.That(actual, Has.No.Property("XYZ").EqualTo(123));

As currently implemented, both of those statements give an error if XYZ doesn't exist. But if we returned a failure, then the first statment would fail, while the second would succeed. The same situation exists for StringConstraints, where the distinction is even more obvious...

Assert.That(123, Does.StartWith("one")); // gives an error
Assert.That(123, Does.Not.StartWith("one")); // also gives an error

So, in any case where we want both the affirmative and the negative assert to "fail", we have to throw.

@jnm2

@jnm2 I believe we are all in violent agreement!

@ChrisMaddock

@ChrisMaddock this issue is in the 3.9 milestone which is fine, but be aware that I will not hold up the release for this issue. I am going to do a quick scan of the open issues, but I think we are close to ready for 3.9. Have you made any progress on this?

Yeah, I'll remove it, it won't happen in time. Thanks!

Navigation Menu

Repository navigation

Improve PropertyConstraint error output #2405

1 remaining item

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Improve PropertyConstraint error output #2405

Description

Activity

CharliePoole commented on Sep 1, 2017

ChrisMaddock commented on Sep 1, 2017

CharliePoole commented on Sep 1, 2017

ChrisMaddock commented on Sep 1, 2017

CharliePoole commented on Sep 1, 2017

ChrisMaddock commented on Sep 1, 2017

jnm2 commented on Sep 2, 2017

CharliePoole commented on Sep 2, 2017

jnm2 commented on Sep 2, 2017

CharliePoole commented on Sep 2, 2017

jnm2 commented on Sep 2, 2017

1 remaining item

CharliePoole commented on Sep 2, 2017

jnm2 commented on Sep 2, 2017

CharliePoole commented on Sep 2, 2017

jnm2 commented on Sep 2, 2017

CharliePoole commented on Sep 2, 2017

jnm2 commented on Sep 2, 2017

ChrisMaddock commented on Sep 4, 2017

jnm2 commented on Sep 4, 2017

CharliePoole commented on Sep 4, 2017

CharliePoole commented on Sep 4, 2017

rprouse commented on Nov 7, 2017

ChrisMaddock commented on Nov 7, 2017

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions