Try.testSucceed() does not fail on a fail

[WJSchakel]

V Try.testSucceed(Assignment<V>, String, Class<T extends Throwable>) is used to check whether an assignment runs. For example, we have a constructor that may throw a NullPointerException. We can check that new ClassName("argument"); indeed runs. The method accepts a throwable class. As the constructor can throw a NullPointerException, we give that class. Should the assignment throw e.g. an IllegalArgumentException, the test should neither succeed or fail, but throw an exception. The test is then improper.

The code reads:

    public static <V, T extends Throwable> V testSucceed(final Assignment<V> assignment, final String message,
            final Class<T> throwableClass)
    {
        V value;
        try
        {
            value = assignment.assign();
        }
        catch (Throwable cause)
        {
            if (throwableClass.isAssignableFrom(cause.getClass()))
            {
                value = null;
            }
            else
            {
                throw new AssertionError("Assignment failed on unexpected Throwable, expected ("
                        + throwableClass.getSimpleName() + "), but got (" + cause.getClass().getSimpleName() + ").");
            }
        }
        return value;
    }

Problem 1 The assignment should run successfully for the test. If the test gets in to the catch, the test has either failed or behaved unexpectedly. If, however, the thrown cause is of an expected type, we just have value = null; and the test succeeds. Instead, it should throw new AssertionError(message); (note that message is not even used in the method).

Problem 2 In the else-block, a normal exception should be thrown, rather than an AssertionError, as the test behaved unexpectedly with a wrong type of cause.

There is a similar method without a return type, and where the first argument is an execution: void Try.testSucceed(Execution, String, Class<T extends Throwable>). This method also suffers from problem 2.

Both methods also have a testFail(...) counterpart, which also suffer from problem 2. In all 4 cases where the message contains failed on unexpected Throwable, AssertionError can simply be replaced with RuntimeException.

[averbraeck]

I can see that testSucceed indeed does not show the correct behavior. The simplest case:

        Double dn = null;
        Double d1 = Double.valueOf(1.0);
        Try.testSucceed(() -> d1.toString());
        Try.testSucceed(() -> dn.toString());

does not throw any errors or exceptions, and of course one of the cases should throw an exception (the dn one)...

On the other hand, testFail does seem to show the correct behavior. See the following code:

        Double dn = null;
        Double d1 = Double.valueOf(1.0);
        Try.testFail(() -> dn.toString());
        try
        {
            Try.testFail(() -> d1.toString());
            System.err.println("NOT ok");
        }
        catch (Throwable e)
        {
            System.out.println("ok");
        }

This prints ok. I tested with Try.testFail(() -> dn.toString()); whether the null pointer assignment indeed throws an exception, and it does. I tested with Try.testFail(() -> d1.toString()); whether it throws and exception, and it does not, so the testFail throws an AssertionError, which is to be expected -- the assertion was that the method call would throw an exception, and it didn't, so it should throw an AssertionError -- the same error that the assert (or assertTrue(), assertEquals()) throws on the failure of an assertion.

[WJSchakel]

testFail shows the wrong behavior if you use the version with 3 arguments, where you explicitly give the exception type that an assignment/execution should fail on. If another exception pops up, the test neither failed nor succeeded, but is improperly defined. I think it should throw an exception to indicate to the programmer that the test is not correctly defined.

[averbraeck]

By the way, I never understood the necessity of the testSucceed(() -> someExecution()) method for unit tests. When you expect something to succeed, just put the execution() in the unit test, and when it does not succeed because of an error or exception, the unit test fails, as it should.

The use case for testFail(() -> someExecution()), on the other hand, is very compelling. Instead of writing:

try
{
    someExecution();
    fail("this execution should have thrown an exception");
}
catch (Exception e)
{
  // expected
}

we can write the one-liner:

testFail(() -> someExecution(), "this execution should have thrown an exception");

So, what is the use-case for testSucceed? The javadoc doesn't give a clue. Actually, now that I look at it, the javadoc does not give any clue about any of the method parameters for testFail and testSucceed... Another opportunity for improvement!

[averbraeck]

testFail shows the wrong behavior if you use the version with 3 arguments

Ok -- clear! I will check that and make improvements. The expected behavior for testFail in that case is that when it fails with the CORRECT exception, testFail succeeds, and when it either DOES NOT fail, or fails with the WRONG EXCEPTION, testFail should throw an AssertionError. Did I understood correctly?

[averbraeck]

By the way, I will also raise this bug in djunits. djunits has a copy of the Try and Throw classes, to not make djunits dependent on external packages.

[WJSchakel]

The expected behavior for testFail in that case is that when it fails with the CORRECT exception, testFail succeeds, and when it either DOES NOT fail, or fails with the WRONG EXCEPTION, testFail should throw anAssertionError`. Did I understood correctly?

Almost:

• No exception pops up: the test fails throw AssertionError
• The right exception pops up: test succeeds, return null for an assignment, do nothing for an execution
• A wrong exception pops up: test itself is improper throw RuntimeException

[WJSchakel]

By the way, I never understood the necessity of the testSucceed(() -> someExecution()) method for unit tests. When you expect something to succeed, just put the execution() in the unit test, and when it does not succeed because of an error or exception, the unit test fails, as it should.

True. The only reason I imagine is that if it fails you get an AssertionError as a proper test result, rather than some random exception. I think tests should fail with an AssertionError, while other exceptions indicate that some code in the test itself is wrong. But I'm not sure about this, and the testSucceed methods are confusing, and I can find only 1 usage.

[averbraeck]

• A wrong exception pops up: test itself is improper throw RuntimeException

This one I don't understand. It should, in my opinion, also throw an AssertionError, but maybe with a more extensive explanation. Note that an Error is more severe than an Exception, so even when there is a try...catch(Exception e) around the testFail() code, the AssertionError will still make the unit test fail (as it should).

Would it be an idea to precede or append the error message (as an AssertionError) with a string "unexpected exception thrown: XxxException instead of YyyException" instead of throwing a RuntimeException?

And we could even precede or append the error message when the assignment or execution did not fail with a string "Assignment of execution unexpectedly did not fail with YyyException".

[averbraeck]

The only reason I imagine is that if it fails you get an AssertionError as a proper test result, rather than some random exception.

I disagree. When you expect a method call or assignment to succeed in your unit test, ANY deviation from that should throw an AssertionError in the end. Period. When you just execute the code as someMethod() and it throws any exception, JUnit will turn that into an AssertionError and fail the unit test. The explanation of the unit test failure provides you with the stack trace, which contains all the information you need. The RuntimeError just adds a layer of obfuscation IMHO.

Another reason for this is that the code of Try.testFail() or Try.testSucceed() could be wrong itself. THAT would never throw an AssertionError, but another type of exception, and therefore is a giveaway that we should look into the code in the Try class to resolve that unexpected exception...

The more I think about it, the more I am in favor of scrapping the testSucceed(), by the way...

[WJSchakel]

Perhaps in the end an AssertionError results, but in the JUnit pane you have 3 results for a test:

• success (green check)
• failure (blue cross)
• error (red cross)

To me the latter two categories mean:

• failure; the tested code is not correct
• error; the test code itself is not correct

But I realize both may happen due to faulty code either in the test code, or tested code. So an AssertionError with additional description is fine.

[WJSchakel]

And in that case indeed the testSucceed methods make no sense. Let's remove them as they are confusing.

[averbraeck]

Agree.

• Let's update the javadoc as well. It is too brief for coders to understand.
• And, of course, the unit tests of the Try class should be updated. Without using Try itself, so in the old fashioned way. Apparently we have not been complete in our tests, since especially the 2-line incorrect testSucceed() code should have been caught...

[averbraeck]

Javadoc has been extensively updated. It now states, e.g.:

<V, T extends Throwable> V org.djutils.exceptions.Try.testFail(Assignment assignment, String message, Class expectedThrowableClass)

Method for unit tests to test if an expected exception is thrown on an assignment. This method provides an explanation message, and it is checking for a specific type of exception to be thrown. The testFail() method throws an AssertionError when the assignment does not throw an exception, or when it throws a different exception than expectedThrowableClass. A way to use the method is, for instance:

   Try.testFail(() -> methodFailsOnNull(null), "call should have thrown an NPE", NullPointerException.class);

or

   Try.testFail(new Try.Assignment<Double>()
   {
       @Override
       public Double assign() throws Throwable
       {
           return methodFailsOnNull(null);
       }
   }, "call should have thrown an NPE", NullPointerException.class);

Type Parameters:    <V> value type, which is the return type of the assignment    <T> throwable type, which ensures that we provide a throwable class as the argument Parameters:    assignment Assignment; functional interface to assign value    message String; message to use in the AssertionError when the test fails    expectedThrowableClass Class; the class of the exception we expect the assignment to throw Returns:    V; assigned value

[averbraeck]

Unit tests for testFail() have been severely extended. There are now around 800 lines of test code for testFail().

[averbraeck]

testSucceed() has been removed from the code and from the unit tests. Issue solved.