Make asserts meaningful

[elharo]

Asserts no longer guarantted to pass.

[pzygielo]

I'm guessing that the true meaning is that mojo.execute() throws. And IMO it'd better to assert that that happens. The non null message seems to be secondary.

[elharo]

Yes, it's secondary but still not great. The real test is the fail on the previous lines.

[pzygielo]

I'd rather see fail there replaced with assertThrows. This would be meaningful.

[elharo]

It means no more or less than this code does. If you want to send that PR, I'll approve it. Otherwise, let's proceed with this change.

[pzygielo]

Proposed change introduces new constraint for the exception message. It wasn't there, wasn't needed then, and isn't needed now.

I think the always true assertion was added to satisfy checkstyle to avoid empty block.

If this PR gets merged, refactoring to come next, would need to consider that. Not only that the exception is thrown (what is currently checked) but also that the message is not null.

IMO, it's unnecessary and it would be better to migrate to assertThrows only.
assertTrue(true) could then be eliminated.
assertNotNull(...) couldn't.

[elharo]

putting in meaningless lines of code to make checkstyle happy is not a good thing.

[pzygielo]

I think using @Test(expected = MojoExecutionException.class) would be better choice.

[slawekjaranowski]

There is no @Test(expected ...) in JUnit 5
For me assertThrows is the best we control which one action generate exception

[pzygielo]

There is no @Test(expected ...) in JUnit 5

maven-scm/pom.xml

Lines 281 to 285 in ad63ac5

	<dependency>
	<groupId>junit</groupId>
	<artifactId>junit</artifactId>
	<version>4.13.2</version>
	</dependency>

[slawekjaranowski]

I know that now ju 4 is used ... 😄

[Bukama]

As @pzygielo has pointed out in comments:
The JUnit 4 way to test for an expected MojoExecutionException is

@Test(expected = MojoExecutionException.class)

instead of a try/catch block

(non binding review)

[elharo]

That's not good and the JUnit project no longer recommends it. The problem is it doesn't indicate which line of code threw the exception so you can get false positives — tests that pass when they shouldn't — when the same exception class gets thrown from somewhere unexpected.

[pzygielo]

The problem is it doesn't indicate which line of code threw the exception so you can get false positives — tests that pass when they shouldn't — when the same exception class gets thrown from somewhere unexpected.

Indeed.

[Bukama]

That's not good and the JUnit project no longer recommends it. The problem is it doesn't indicate which line of code threw the exception so you can get false positives — tests that pass when they shouldn't — when the same exception class gets thrown from somewhere unexpected.

The code line assertNotNull(e.getMessage()); does not check for a specific line of code either. You should then check for a specific exception message.

And I know that the way to test assertions in JUnit Jupiter is different.

[elharo]

The code line assertNotNull(e.getMessage()); does not check for a specific line of code either. You should then check for a specific exception message.

No, that's done with the try block.