Make asserts meaningful #232

elharo · 2024-12-12T13:26:56Z

Asserts no longer guaranteed to pass.

pzygielo · 2024-12-12T13:41:01Z

maven-scm-plugin/src/test/java/org/apache/maven/scm/plugin/ChangeLogMojoTest.java

            fail("mojo execution must fail.");
        } catch (MojoExecutionException e) {
-            assertTrue(true);
+            assertNotNull(e.getMessage());


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.

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

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

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.

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.

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

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

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

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>

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 · 2024-12-25T21:24:48Z

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 · 2024-12-25T21:42:24Z

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 · 2024-12-26T07:25:39Z

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 · 2024-12-26T12:46:30Z

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.

Make asserts meaningful

9853f5e

elharo requested a review from slawekjaranowski December 12, 2024 13:26

pzygielo reviewed Dec 12, 2024

View reviewed changes

elharo marked this pull request as ready for review December 12, 2024 14:02

Bukama requested changes Dec 25, 2024

View reviewed changes

slawekjaranowski approved these changes Jan 9, 2025

View reviewed changes

elharo merged commit 1487251 into master Jan 9, 2025
37 checks passed

elharo deleted the true branch January 9, 2025 20:53

kwin added enhancement New feature or request maintenance and removed enhancement New feature or request labels Aug 6, 2025

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

Make asserts meaningful #232

Make asserts meaningful #232

Uh oh!

Conversation

elharo commented Dec 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bukama left a comment

Choose a reason for hiding this comment

Uh oh!

elharo commented Dec 25, 2024

Uh oh!

pzygielo commented Dec 25, 2024

Uh oh!

Bukama commented Dec 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elharo commented Dec 26, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

elharo commented Dec 12, 2024 •

edited

Loading

Bukama commented Dec 26, 2024 •

edited

Loading