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

custom error messages #35

Closed
benjamine opened this issue Oct 31, 2011 · 6 comments
Closed

custom error messages #35

benjamine opened this issue Oct 31, 2011 · 6 comments

Comments

@benjamine
Copy link

It would be nice if we can provide custom messages to assertions, eg:

Browser.ContainsText(text).ShouldBe(true, "'"+text+"' not found on page body");

or:

Browser.ContainsText(text).ShouldBe(true).Otherwise("'"+text+"' not found on page body");

(in the example ContainsText is an implementation that cannot be replaced by ShouldContain() )

@xerxesb
Copy link
Member

xerxesb commented Nov 1, 2011

Hi Ben - thanks for your suggestion :)

My brief thoughts on your idea are that Shouldly's intention is to provide more helpful error messages out of the box so that you don't (necessarily) have to.

By grabbing the source-line when we print the error message, we're intending that the code is clear enough such that the reason for the failure is obvious.

In your particular case, i'm not sure that the user-provided message is giving any more information about the reason for the failure than you could ascertain from Shouldly's failure message alone.

e.g. Browser.ContainsText(text).ShouldBe(true) will fail with the message:

    Browser.ContainsText(text)
should be
    true
but was
    false

which is as much information as "text not found on page body".

If you wanted more detail about the text, you could (say) rename the text variable to something which is closer to its actual meaning, like hrefElementForContactUsPage

So there's that option, or another possibility is that your example has shown a deficiency in Shouldly's ability to cater for custom text matching. Out of curiosity, what does your ContainsText method do differently?

@benjamine
Copy link
Author

Ops, I didn't noticed that the failing line will be included in the error message. Of course, I agree with the philosophy of no string messages needed.
I can't give text a better name because it's inside a generic method, would it be too ridiculous to ask for printing variable values along with message? eg:

    Browser.ContainsText(text)
should be
    true
but was
    false

variables: text: "a string value"

(just a thought, it sounds hard, but if it's possible I can help with it on a fork)

about the ContainsText() method, I'm no sure (it's in a third-party lib), but I think it searches thru a page DOM tree for text that is visible to user. Also I couldn't use ShouldContain because it prints the whole page text, which is too long.

I think I can close this issue now,
Thanks again!

@xerxesb
Copy link
Member

xerxesb commented Nov 3, 2011

Hi Ben - you closed the issue, but I cant help but feel I wasn't able to fully solve your problems with Shouldly..

Are you satisfied with the limitation of actual output of 100 chars and would that solve the problems you're experiencing?

-xerx

@benjamine
Copy link
Author

Hi, yes, absolutely!
ps: I googled a bit about printing a sort-of "Quick Watch" with variables in the failing line, and verified that it's insanely complicated (probably involves assembly weaving! :P), so I think Shouldly is doing great, now I should:

  • Replace ContainsText() with a method that extracts the "user-visible text in page body", and apply ShouldContain on that, or
  • Create a custom extension method "ShouldContainInBody", using a try{} catch{} to print a more custom messages. Knowing of course, that this should be pretty rare and avoided when posible.

Thanks for you concern xerxes.

@troelsrichter
Copy link

I'm still missing additional assertion error message and keeps me from using shouldly in some scenarios. Mostly in situations where it isn't clear why the assertion is there in the first place. So actually it is more a reason why the assertion is there. I think a syntax like this would be useful.

        detailResult.Papers[0].General.ShouldBe(null).Because(
            "General should not be used in this context, but still visible for backwards compatibility");

@benjamine
Copy link
Author

First of all I think that syntax is not possible, as the ShouldBe() method will throw an Exception before the Because() method is even called.
But I think is by-design the intention of providing an expressive interface so custom messages are not needed.
Maybe you cand do:

// check for an ObsoleteAttribute using reflection (is it possible or using a lambda is required?)
detailResult.Papers[0].General.ShouldBeObsolete();

// if you also want to be sure that obsolete properties are never assigned, this could check that property value === default(T)
detailResult.Papers[0].General.ShouldBeObsolete().AndUnassigned();

You can write another issue requesting for that (and possibly submit a pull request with it).

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