Add Assert.throws

[ashes999]

As discussed in #113

[ashes999]

@mikestead did you have a chance to look at this?

[mikestead]

A few points

• You're not passing posinfos so when it fails the line of failure in the test wont be correct
• This seems to catch any exception and not a specific one you expect (like a specific string or class) so it's impossible to know if what was thrown was what you were expecting. i.e. the test may look lke its passing but shouldn't, e.g. if code was null or not a function
• The indentation doesn't match the rest of the file

[ashes999]

• I'll take a look. I didn't know about posinfo.
• You're not specifying an expected exception any more. throws returns the caught exception. The common use-case is to call something like var caught = Assert.throws(...) and then make further assertions against caught. This also keeps the framework a little leaner.
• It looks like you're using tabs (I thought everyone uses spaces because of cross-IDE consistency). I'll tabify my indentation.

[ashes999]

How does it look now @mikestead ?

[ashes999]

One issue I find is that Haxe doesn't predict the type correctly.

This fails (simplified example):

var message = Assert.throws(function() { 
  throw "error";
});

The type of message isn't String, it's empty(TClass). If I write naive code like:

Assert.isTrue(message.indexOf("error") > -1);

This fails at runtime with an Invalid call. It's not obvious why.

The work-around is pretty simple -- just attach a type to message:

var message:String = ...

Then everything works as expected.

[mikestead]

This still suffers the same issue, as you're catching everything that's thrown. I think we should be explicitly asserting that an expected error was thrown.

function throws(expected:Dynamic, func:Dynamic, ?info:PosInfo)
{
    var actual = null;
    try func()
    catch(e:Dynamic) actual = e;

    if (Std.is(expected, String)) Assert.areEqual(expected, actual, info);
    else Assert.isType(actual, expected, info);
    return actual;
}

Assert.throws('some string', function() { throw 'some string'; });
Assert.throws(SomeError, function() { throw new SomeError(); });

[ashes999]

I think it's better to have a simpler throws method. The user just has to narrow down which line of code should throw. If they care about the type, they can write their own if/else code (like you've described) to test the type more specifically.

Plus, with string errors, the code looks a little uglier:

var message:String = Assert.throws(String, function() { ... })
Assert.areEqual("Invalid data", message);

Instead of this:

var message:String = Assert.throws(function() { ... })
Assert.areEqual("Invalid data", message);

I don't see a big difference in terms of code clarity between these two options, either.

If you insist, let me know, and I'll change the method to what you've described.

[mikestead]

The inspo for this new method I believe comes from c#? From what I can see specifying the type of exception to be thrown is core to its usage, otherwise you always have to write two assertions. The first Assert.throws.. and the second to assert that the correct thing was thrown. Only doing the former means you could be catching the wrong exception and never know.

[ashes999]

How does it look now?

[mikestead]

Better, but you're checking for a specific type so inheritance / interfaces are ignored, which I don't think they should be if you just care about it being some base type.

[ashes999]

Ah, I didn't realize you used Std.is. I'll update my code.

[ashes999]

Updated. Anything else?

[mikestead]

Hey @ashes999 sorry for the delay, yes this looks better thanks for the work.

Can you fix up all the indentation issues and then we can get this in. See https://github.com/ashes999/MassiveUnit/blob/master/src/massive/munit/Assert.hx and https://github.com/ashes999/MassiveUnit/blob/master/test/massive/munit/AssertTest.hx

[ashes999]

Is that better @mikestead ?

[mikestead]

👍

[Leonix]

One thing I noticed is that throws() should probably increment assertionCount like all other methods in this class.

[ashes999]

@Leonix that's a very astute observation. Maybe you can open a separate PR for it? This PR was closed eight months ago.