add expected Exception type to assert.throwsException

[asanzo]

something like:

assert.throwsException(MyError, { a.method1() })

[asanzo]

BTW, diabolic issue.

[Juancete]

The solution that was implemented accepts something like this:

assert.throwsException(new MyError(), { a.method1() })

it's fine?

[fdodino]

:+1:

[matifreyre]

Does that implementation take into consideration any of the exception's attributes, like the associated text?

[fdodino]

I was also needing this:

assert.throwsException("Cannot send message + to null", { a.method() })

So compare message or exception thrown with or without message (optional if it is not present) would be nice!

[Juancete]

@fdodino and @matifreyre Initially only compares the exception class. My idea will work like this:

assert.throwsException(new BusinessException(), { => throw new BusinessException() })

Works fine. Only checks the exception class, but if you do

assert.throwsException(new BusinessException("lalala"), { => throw new BusinessException() })

Fails because an error message is seted and is not equals. It will be fine if you do something like

assert.throwsException(new BusinessException("lalala"), { => throw new BusinessException("lalala") })

is that ok?

[matifreyre]

I'm more worried about a case such as this:

assert.throwsException(new BusinessException(), { => throw new BusinessException("lalala") })

Will it succeed?

On Wed, Jul 6, 2016 at 8:30 PM, Juancete notifications@github.com wrote:

@fdodino https://github.com/fdodino and @matifreyre https://github.com/matifreyre Initially only compares the exception class. My idea will work like this:

assert.throwsException(new BusinessException(), { => throw new BusinessException() })

Works fine. Only checks the exception class, but if you do

assert.throwsException(new BusinessException("lalala"), { => throw new BusinessException() })

Fails because an error message is seted and is not equals. It will be fine if you do something like

assert.throwsException(new BusinessException("lalala"), { => throw new BusinessException("lalala") })

is that ok?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/uqbar-project/wollok/issues/666#issuecomment-230937954, or mute the thread https://github.com/notifications/unsubscribe/AJ-EZg2TzWX4A7_b7_cnQeb8d-t15sMeks5qTDp7gaJpZM4IpIvW .

[Juancete]

Yes, it's confusing. In that case only checks the exception type. So it will succeed. Because i think "I want to test if i get the same exception class, and if i set an error message in the expected object, i'll check it too.."

if both are equals when have the same class and error message then

assert.throwsException(new BusinessException(), { => throw new
BusinessException("lalala") })

fails and

assert.throwsException(new BusinessException("lalala"), { => throw new
BusinessException("lalala") }) 

it's ok

[asanzo]

I like the approach of comparing both type and message, yet i find it unnerving having this succeed:

assert.throwsException(new BusinessException(), { => throw new BusinessException("lalala") })

Why not just do a deep equality? That is quite simple, easy to explain and to understand. And it will draw attention to both Exception type & description. If I have to teach Exceptions to someone who doesn't know a thing about them, I'd go for this for simplicity.

I mean, I want them to focus on testing exceptions, and knowing that exceptions commonly carry messages. The deep equality approach, as I see it, helps this objective.

These succed:

assert.throwsException(new BusinessException(),{ => throw new BusinessException() } )
assert.throwsException(new BusinessException("lalala"),{ => throw new BusinessException("lalala") } )

These don't:

assert.throwsException(new BusinessException("lalala"),{ => throw new BusinessException() } )
assert.throwsException(new BusinessException(),{ => throw new BusinessException("lalala") } )
assert.throwsException(new BusinessException(),{ => throw new HellNoException() } )
assert.throwsException(new BusinessException("lalala"),{ => throw new HellNoException("lalala") } )

Simple, direct. And adding Fer's feature would simply mean having a different assert method, where

these succeed:

assert.throwsExceptionMessage("lalala",{ => throw new BusinessException("lalala") } )
assert.throwsExceptionMessage("lalala",{ => throw new HellNoException("lalala") } )

and these don't:

assert.throwsExceptionMessage("lalala",{ => throw new BusinessException() } )
assert.throwsExceptionMessage("lalala",{ => throw new BusinessException("helll nooooo") } )

[Juancete]

@asanzo groso!!!!

Yes, it's the mos simple and razonable way to implement the two features :D :+1:

[matifreyre]

Well, I´m not so sure about the first case. Having this succeed:

assert.throwsException(new BusinessException(), { => throw new BusinessException("lalala") })

Is the normal path in most languages I know. I mean, you don't need to actually create an exception object, but you only specify its class and not its content. The problem I see with your approach is the following... What if the content is not just a message but has some information about what's happening underneath (e.g. a new object that was created within the method being tested)? You'd need to replicate the entire object in the test too to be able to compare it.

In any case, we could use 3 different messages... I'm not sure about the names, but they could be:

• throwsException (compares everything)
• throwsExceptionType (or class)
• throwsExceptionMessage

On the other hand, I don't want to extend the assert WKO too much :-s

Opinions?

On Thu, Jul 7, 2016 at 9:52 AM, Juancete notifications@github.com wrote:

@asanzo https://github.com/asanzo groso!!!!

Yes, it's the mos simple and razonable way to implement the two features :D 👍

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/uqbar-project/wollok/issues/666#issuecomment-231068474, or mute the thread https://github.com/notifications/unsubscribe/AJ-EZnbOgiKkF7d5uSmNaphcCXSVrkkRks5qTPaZgaJpZM4IpIvW .

[asanzo]

First of all, "Ok to all". I mean, let's go for whatever everyone wants :)

But, if I may say something: Again, I'd normally go for the solution that makes learning easier, and the all-comparing one seemed simple enough to understand. I see the problem for more complex exceptions, but I didn't know that that feature was being used to teach. However if passing objects through exceptions is something that is rare, Why not stick to the simple solution?

I just want to keep thinking in what I would use to teach and not how to make the language more flexible.

The ideal case for me would be:

assert.throwsExceptionType(BusinessException, { => throw new
BusinessException("lalala") })

Because 1) I wouldn't want the student to involve in deeper details, 2) it is simple enough to understand, and 2) it is what we are accustomed to in the industry.

However, as we are obligued to instantiate the class -we are, right?-, my ideal changes to the one I described above.

assert.throwsException(new BusinessException("lalala"), { => throw new
BusinessException("lalala") })

Anyway, I think that the three messages would satisfy everyone. Why wouldn't we want to grow the assert object? If we are in the path of building Wollok for "every teacher need", I see it as something good. If , however, we wanted to impose a teaching strategy, I agree that the assert object needs a smaller interface. In the making of Gobstones, for example, they took decisions so that there is only one way to do what you want to do, and it is in the way that I want you to do it, because I find it important.

[javierfernandes]

assert.throwsExceptionType(BusinessException, { => throw new BusinessException("lalala") })

Doesn't force you to instantiate the class, just reference it.

Of course if they haven't seen classes yet, it will be something like "do it like this right now, you'll understand later". Which is not quiet good.

But, if that's the case, then I think that the example is wrong, and the tested code would't have any specific exception (new exception). but problaly:

• using one of the user's object calling "self.error(...)"
• using a wollok sdk class which throws any exception.

In anycase, probably the user doesn't really matter about the exception class. So in that case he will use the method to assert "any exception raised" If he want more control, or more specific assert, then use

assert.throwsExceptionWithMessage("Pepita se murio", { ... } )

So, for me it is not flexibility for crazy advanced developers, you really need different asserts for the different context of the same OOP subject.

To summarize in order

-

assert.throwsException { ... } // doesn't care which one. Ok.. could be weird :P

-

assert.throwsExceptionWithMessage("Pepita se murio", { ... } )

-

assert.throwsException(PepitaException, "Pepita se murio", { ... } )

I would think that 90% of the times they will just use the second method.

What I really think is "wrong" is

assert.throwsException(*new PepitaException("Pepita se murio")*, { ... } )

Which creates a new exception just to compare it ?

I don't know, it is weird. I'm not saying it is a matter of memory consumption or things like that. Just that, it is weird.

Like instantiating an object to use it just once

var volado = new Pajaro().volar()

Ok, sometimes happens in a practice, in real world. But it is usually because of a problem in the design.

My 2 cents

On Fri, Jul 15, 2016 at 1:44 PM, asanzo notifications@github.com wrote:

First of all, "Ok to all". I mean, let's go for whatever everyone wants :)

But, if I may say something: Again, I'd normally go for the solution that makes learning easier, and the all-comparing one seemed simple enough to understand. I see the problem for more complex exceptions, but I didn't know that that feature was being used to teach. However if passing objects through exceptions is something that is rare, Why not stick to the simple solution?

I just want to keep thinking in what I would use to teach and not how to make the language more flexible.

The ideal case for me would be:

assert.throwsExceptionType(BusinessException, { => throw new BusinessException("lalala") })

Because 1) I wouldn't want the student to involve in deeper details, 2) it is simple enough to understand, and 2) it is what we are accustomed to in the industry.

However, as we are obligued to instantiate the class -we are, right?-, my ideal changes to the one I described above.

assert.throwsException(new BusinessException("lalala"), { => throw new BusinessException("lalala") })

Anyway, I think that the three messages would satisfy everyone. Why wouldn't we want to grow the assert object? If we are in the path of building Wollok for "every teacher need", I see it as something good. If , however, we wanted to impose a teaching strategy, I agree that the assert object needs a smaller interface. In the making of Gobstones, for example, they took decisions so that there is only one way to do what you want to do, and it is in the way that I want you to do it, because I find it important.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/uqbar-project/wollok/issues/666#issuecomment-233004165, or mute the thread https://github.com/notifications/unsubscribe-auth/AEORWMpcogZVirf8aWNeuenxKoTU9bvvks5qV7jqgaJpZM4IpIvW .

[matifreyre]

We don't have the ability to pass a class or classname as a parameter, right? I mean, we can always pass a classname, but it cannot be resolved in the method.

[javierfernandes]

ja.. good point :P

I forgot about that :(

On Fri, Jul 15, 2016 at 3:11 PM, Matías Freyre notifications@github.com wrote:

We don't have the ability to pass a class or classname as a parameter, right? I mean, we can always pass a classname, but it cannot be resolved in the method.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/uqbar-project/wollok/issues/666#issuecomment-233026258, or mute the thread https://github.com/notifications/unsubscribe-auth/AEORWFu0NPtVHnSbrmDyR9i_8MThssRYks5qV81DgaJpZM4IpIvW .

[asanzo]

Yeah, I also don't like instantiating. And sir, you have a great point with the problem of testing in "just objects" moments...

So all we can do now is:

// For initial i-dont-know-classes-yet testing
assert.throwsException({ => throw new BusinessException() } ) 

// equals comparing exception
assert.throwsExceptionLike(new BusinessException("lalala"),{ => throw new BusinessException("lalala") } 

// check the message, no matter the type: (also useful for  initial i-dont-know-classes-yet testing )
assert.throwsExceptionWithMessage("lalala", { => throw new BusinessException("lalala") } )

// check the type, no matter the message:
assert.throwsExceptionWithType(new BusinessException(),{ => throw new BusinessException("lalala") } )

[fdodino]

Agree!

[matifreyre]

So do I. It's 4 methods in the end... @Juancete you'll handle this, right?

[npasserini]

I buy it as a temporal solution, but we should avoid instanciating an exception just for that... it is a bad programming practice that we are teaching to all students. (Yet I prefer this option much rather than the "by example" proposal.)

Adding class references could be a possibility... yet being able to expressing classes as values in your code puts us un step less away from thisObject.class() = thisClass. But being careful it might be the way to go, and at least it is consistent with the way that we treat catch-blocks.

It would be nice to come up with something else, in which exceptions are not treated in a type-based maner... this is kind-of inconsistent with the rest of the language design. I wander what happens if you allow to throw any value.

On Fri, Jul 15, 2016 at 9:58 PM, Matías Freyre notifications@github.com wrote:

So do I. It's 4 methods in the end... @Juancete https://github.com/Juancete you'll handle this, right?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/uqbar-project/wollok/issues/666#issuecomment-233055312, or mute the thread https://github.com/notifications/unsubscribe-auth/AEa1OdsFvbcNr75xG_hhxYpSmjSZOZHWks5qV-Z6gaJpZM4IpIvW .

[javierfernandes]

In terms of implementation, it is like you cannot throw a non-exception object, just because of an arbitrary static check. At the beginning you were able to throw any object.

On Fri, Jul 15, 2016 at 7:03 PM, Nico Passerini notifications@github.com wrote:

I buy it as a temporal solution, but we should avoid instanciating an exception just for that... it is a bad programming practice that we are teaching to all students. (Yet I prefer this option much rather than the "by example" proposal.)

Adding class references could be a possibility... yet being able to expressing classes as values in your code puts us un step less away from thisObject.class() = thisClass. But being careful it might be the way to go, and at least it is consistent with the way that we treat catch-blocks.

It would be nice to come up with something else, in which exceptions are not treated in a type-based maner... this is kind-of inconsistent with the rest of the language design. I wander what happens if you allow to throw any value.

On Fri, Jul 15, 2016 at 9:58 PM, Matías Freyre notifications@github.com wrote:

So do I. It's 4 methods in the end... @Juancete https://github.com/Juancete you'll handle this, right?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub < https://github.com/uqbar-project/wollok/issues/666#issuecomment-233055312 , or mute the thread < https://github.com/notifications/unsubscribe-auth/AEa1OdsFvbcNr75xG_hhxYpSmjSZOZHWks5qV-Z6gaJpZM4IpIvW

.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/uqbar-project/wollok/issues/666#issuecomment-233080638, or mute the thread https://github.com/notifications/unsubscribe-auth/AEORWLsFiD4oiBx1xttoNuH3gdTajqm9ks5qWAPBgaJpZM4IpIvW .