[sinttest] Add AbstractSmackIntTest.assertResult()

[Flowdalic]

Counter proposal to #582

[guusdk]

When an assertion fails, it should throw an java.lang.AssertionError or possibly org.opentest4j.AssertionFailedError, not any Exception. This allows the testing framework to distinguish between tests that fail because of a bug in the system under test, or because of a bug in the test implementation itself.

Returning a value in an assertion is an anti-pattern. An assertion should only do that: assert something - not also produce a value that is expected to be used in follow-up code (arguably, asserting something is the last thing in a test anyway, as they should be concise).

[Flowdalic]

When an assertion fails, it should throw an java.lang.AssertionError or possibly org.opentest4j.AssertionFailedError, not any Exception.

Right, I noticed that sitting on the couch too. See updated revision.

Returning a value in an assertion is an anti-pattern. An assertion should only do that: assert something - not also produce a value that is expected to be used in follow-up code (arguably, asserting something is the last thing in a test anyway, as they should be concise).

What would be the argument for such an artificial limitation? Seems pointless to declare it an anti-pattern when it is clearly useful. It is also established practice, see, for example, assertThrows().

That said, you are free to simply ignore anything that the function returns.

[guusdk]

Note that a 'timeout' also is an assertion failure - it might even be be the primary assertion done when this class is used, as validation of the returned value often is not done at all.

[guusdk]

I've refactored #582 to include the changes proposed by you here, adjusted for my feedback.

[Flowdalic]

I've refactored #582 to include the changes proposed by you here, adjusted for my feedback.

I could get maybe convinced to change the result type from SimpleResultSyncPoint to void. But, as we both discovered, this can be sorted out later.

On the other hand, I am sorry but I am confident that wrapping the TimeoutException into AssertionFailure is neither sensible nor provides any benefits. At least, I haven't heard convincing arguments for doing so. Therefore, I am more than inclined to merge this PR instead of #582.

[guusdk]

I guess we just don't agree on the AssertionError thing. Please just commit it as you see fit. I can always create a different variant of the ResultSync stuff in our code, if this turns out to be problematic in my context.

For posterity, as the following was exchanged in a chat that even I can't find anymore:

The test execution frameworks can/will report differently on a test execution problem, depending on the exception that's being thrown is or isn't an AssertionError (or descendant), as shown in the code below (I've added the output of the test run as code comment).

For my purpose, it's important to flag as accurately as possible if a test execution is failing because the system under test is misbehaving, or because the test implementation has a bug - especially so since in our context, people that execute the tests are different people than Smack/SINT developers. That makes it more important to accurately identify the source of the test execution problem: in case of an assertion failure, the person executing the test should start to investigate their own system under test. In case of a test bug, they should file a report with our project.

When SimpleResultSyncPoint's waitForResult() throws a TimeoutException, it is (by far) more likely that this is caused by a bug in the system under test. We can certainly think of ways to causeTimeoutException to be thrown as a result of a bug in the implementation of the test itself, but those will be outliers at best. I therefor remain of the opinion that rethrowing this particular instance of TimeoutException wrapped in an AssertionError is the more correct thing to do.

public class TempTest
{
    @org.junit.jupiter.api.Test
    public void testFail() throws Exception {
        // Result displayed as an 'cross' icon, alt-text "Assertion failure"
        org.junit.jupiter.api.Assertions.fail();
    }

    @org.junit.jupiter.api.Test
    public void testEquals() throws Exception {
        // Result displayed as an 'cross' icon, alt-text "Assertion failure"
        org.junit.jupiter.api.Assertions.assertEquals("foo", "bar");
    }

    @org.junit.jupiter.api.Test
    public void testThrowAssertionError() throws Exception {
        // Result displayed as an 'cross' icon, alt-text "Assertion failure"
        throw new AssertionError();
    }

    @org.junit.jupiter.api.Test
    public void testThrowOpenTest4jAssertionFailedError() throws Exception {
        // Result displayed as an 'cross' icon, alt-text "Assertion failure"
        throw new org.opentest4j.AssertionFailedError();
    }

    @org.junit.jupiter.api.Test
    public void testThrowTimeoutException() throws Exception {
        // Result displayed as an 'exclamation mark' icon, alt-text "Unexpected exception"
        throw new java.util.concurrent.TimeoutException();
    }

    @org.junit.jupiter.api.Test
    public void testThrowIllegalArgumentException() throws Exception {
        // Result displayed as an 'exclamation mark' icon, alt-text "Unexpected exception"
        throw new IllegalArgumentException();
    }

    @org.junit.jupiter.api.Test
    public void testThrowNullPointerExceptionException() throws Exception {
        // Result displayed as an 'exclamation mark' icon, alt-text "Unexpected exception"
        throw new NullPointerException();
    }

    @org.junit.jupiter.api.Test
    public void testDivisionByZero() {
        // Result displayed as an 'exclamation mark' icon, alt-text "Unexpected exception"
        org.junit.jupiter.api.Assertions.assertEquals(999, 2/0);
    }
}

[Flowdalic]

in case of an assertion failure, the person executing the test should start to investigate their own system under test. In case of a test bug, they should file a report with our project.

When SimpleResultSyncPoint's waitForResult() throws a TimeoutException, it is (by far) more likely that this is caused by a bug in the system under test.

I totally agree.

Where we seem to disagree is where the boundary between "test failed due to the system under test failed" and "test failed for other reasons, like out of memory" is drawn. You seem to argue that it is AssertionError exception vs (all?) other exceptions. However, I believe it is exceptions subclassing RuntimeException, Error or NullPointerException vs all other exceptions. The first exception set describes any sort of unexpected runtime error, while all other exceptions are a strong indication (and under the assumption that the test itself is correct) of a misbehaving system.

You also do not seem to be fully convinced of your argumentation, because when I asked you if XMPPErrorException should also be wrapped into AssertionError I got no definite answer. Following your rationale, I believe it must also be wrapped (again, always assuming that the test itself is not flawed).

But then, you end up with code wrapping exceptions, XMPPErrorException is by far not the only example, in sinttest tests. Which can get easily ugly, and even worse, opens the opportunity to miss a case.

That is one of the main reasons I dislike the idea to wrap things in AssertionError. Java's exception class hierarchy is already sufficient for the job. Hence this was baked in early in sinttest.