[SUREFIRE-1556] fail fast on empty element names

[elharo]

@Tibor17 Likely not a full fix, but it should help locate the root cause.

[michael-o]

Why not use the expected property in the annotation?

[elharo]

@Expected was a bad design now replaced in JUnit 4.13 with assertThrows. The problem is that @Expected captures unexpected exceptions thrown from different parts of the method body and thus hides test failures.

[Tibor17]

You can use ExpectedException which is the Rule in JUnit4.

[elharo]

Same issue. This is not a full replacement for the pattern below and risks false negatives. See https://github.com/junit-team/junit4/wiki/Exception-testing

[Tibor17]

@elharo
I am a practicle man. You can of course use [1] with anonymous class instead of Lambda but you can use a traditional ExpectedException with the old version of JUnit.
[1]: https://junit.org/junit4/javadoc/4.13/org/junit/Assert.html#assertThrows(java.lang.String,%20java.lang.Class,%20org.junit.function.ThrowingRunnable)

import org.junit.rules.ExpectedException;

import static org.hamcrest.Matchers.equalTo;

    @Rule
    public final ExpectedException e = ExpectedException.none();

    @Test
    public void testNoStartTag() throws IOException
    {
        e.expect( IllegalArgumentException.class );
        e.expectMessage( equalTo( "Element name cannot be empty" ) );
        writer.startElement( "" );
    }

[elharo]

That has the same problem. It does not guarantee the expected exception was thrown by the right method. JUnit has rightly backed away from this style of testing exceptions.

[Tibor17]

@elharo
In practice the last line with main code must be followed by e.expect. So yes there is guarantee. It is clear that only that one line has to throw the exception. If it does not, then the ExpectedException fails the build. That's why it was designed in JUnit4. You can use the new assertThrows but it has another meaning - wrapping the exception and checking a new errors or exceptions.

[elharo]

This works and it follows the recommendation of the JUnit Wiki and the design of JUnit from 4.13 onward. I'd like to move ahead with fixing this bug. Can I get an approval for this?

[MartinKanters]

@elharo Is this PR still valid and needed? We're looking to release maven-shared-utils, and I'm wondering if we should include this.

[elharo]

Yes, I believe this PR is still valid and useful though it looks like I need to merge in master. Give me a minute.

[elharo]

PTAL

[elharo]

This works and it follows the recommendation of the JUnit Wiki and the design of JUnit from 4.13 onward. I'd like to move ahead with fixing this bug. Can I get an approval for this?

[MartinKanters]

@elharo Hey, you got your approval, but we can't merge anymore due to conflicts. Can you please rebase on master and squash commits?