Use PreconditionAssertions wherever feasible

[neronsoda]

Issue: #4943

Overview

• Refactor tests to consistently use PreconditionAssertions, replacing remaining uses of PreconditionViolationException.class.

Summary

• ✅ 71 files were successfully refactored to replace PreconditionViolationException.class with PreconditionAssertions.
• ⚠️ 7 files still contain direct references to PreconditionViolationException.class.
  Due to their structural constraints, refactoring them to use PreconditionAssertions is not straightforward,
  so they have been intentionally left unchanged.

Please review the 7 files below to confirm whether keeping them as-is is acceptable.

Files Not Refactored (7)

1. jupiter-tests/src/test/java/org/junit/jupiter/engine/discovery/DiscoverySelectorResolverTests.java
2. jupiter-tests/src/test/java/org/junit/jupiter/engine/extension/CloseablePathTests.java
3. jupiter-tests/src/test/java/org/junit/jupiter/engine/extension/ExtensionRegistrationViaParametersAndFieldsTests.java
4. jupiter-tests/src/test/java/org/junit/jupiter/engine/extension/ProgrammaticExtensionRegistrationTests.java
5. jupiter-tests/src/test/java/org/junit/jupiter/engine/extension/TempDirectoryTests.java
6. jupiter-tests/src/test/java/org/junit/jupiter/engine/extension/TimeoutExtensionTests.java
7. jupiter-tests/src/test/java/org/junit/jupiter/params/ParameterizedTestIntegrationTests.java

---

I hereby agree to the terms of the JUnit Contributor License Agreement.

---

Definition of Done

• There are no TODOs left in the code
• Method preconditions are checked and documented in the method's Javadoc
• Coding conventions (e.g. for logging) have been followed
• Change is covered by automated tests including corner cases, errors, and exception handling
• Public API has Javadoc and @API annotations
• Change is documented in the User Guide and Release Notes

[sbrannen]

Thanks for the PR!

I requested a few categories of changes, and I'll try to find time to look into the 7 files you mentioned.

[sbrannen]

Please review the 7 files below to confirm whether keeping them as-is is acceptable.

I think it's fine to leave those 7 files as-is, especially the ones that make use of the EngineTestKit. 👍

[neronsoda]

I’ll commit the changes again as soon as I can after applying all the suggestions you mentioned above :)

I have one additional question.
In the PackageNameFilterTests.java file, there are several cases where the message ~ must not be null or empty is used, for example:

@SuppressWarnings("DataFlowIssue")
@Test
void includeClassNamePatternsChecksPreconditions() {
	assertPreconditionViolationFor(() -> ClassNameFilter.includeClassNamePatterns((String[]) null)).withMessage(
		"patterns array must not be null or empty");
	assertPreconditionViolationFor(() -> ClassNameFilter.includeClassNamePatterns(new String[0])).withMessage(
		"patterns array must not be null or empty");
	assertPreconditionViolationFor(
		() -> ClassNameFilter.includeClassNamePatterns(new String[] { null })).withMessage(
			"patterns array must not contain null elements");
}

Also, in the CsvFileArgumentsProviderTests.java file, there is a case where the message ~ must not be empty is used, for example:

@Test
void throwsExceptionIfResourcesAndFilesAreEmpty() {
	var annotation = csvFileSource()//
			.resources()//
			.files()//
			.build();

	assertPreconditionViolationFor(
		() -> provideArguments(new CsvFileArgumentsProvider(), annotation).toArray()).withMessageContaining(
			"Resources or files must not be empty");
}

Currently, the PreconditionAssertions.java file does not have a helper method for the must not be null or empty and must not be empty case:

public static void assertPreconditionViolationNotNullFor(String name, ThrowingCallable throwingCallable) {
	assertPreconditionViolationFor(throwingCallable).withMessage("%s must not be null", name);
}

public static void assertPreconditionViolationNotNullOrBlankFor(String name, ThrowingCallable throwingCallable) {
	assertPreconditionViolationFor(throwingCallable).withMessage("%s must not be null or blank", name);
}

Would it be acceptable to add the following method and use it for these cases?

public static void assertPreconditionViolationNotNullOrEmptyFor(String name, ThrowingCallable throwingCallable) { 
	assertPreconditionViolationFor(throwingCallable).withMessage("%s must not be null or empty", name); 
}

public static void assertPreconditionViolationNotEmptyFor(String name, ThrowingCallable throwingCallable) {
	assertPreconditionViolationFor(throwingCallable).withMessage("%s must not be empty", name);
}

Also, I found that there are some tests using the message must not be blank.
Should I only refactor the existing cases for must not be null and must not be null or blank?

Or would it be acceptable to create a new helper method for this as well?

[sbrannen]

I’ll commit the changes again as soon as I can after applying all the suggestions you mentioned above :)

👍

Would it be acceptable to add the following method and use it for these cases?

Yes, definitely. The whole point of this exercise is to ensure consistent and simplified use of assertions for precondition violations.

Also, I found that there are some tests using the message must not be blank. Should I only refactor the existing cases for must not be null and must not be null or blank?

Or would it be acceptable to create a new helper method for this as well?

If you find that there is duplication, feel free to introduce new global assertion methods and use them.

[neronsoda]

I just made two commits.
Before committing and pushing, I ran a rebase to sync my branch with the latest remote changes, which pulled in two additional commits that I did not make:

• Update github/codeql-action action to v3.30.6
• Update plugin develocity to v4.2.1

Is this acceptable? My intention was simply to bring the branch up to date before adding my work.

If there’s any problem with this approach, please let me know—and if so, I’d appreciate guidance on how to resolve it.
This is my first time contributing to this project, and I’m still getting comfortable with Git commands; apologies for missteps.

[sbrannen]

This has been merged into main in 1312cda and revised in 7898c56.

Thanks so much the contribution! 👍

[sbrannen]

Is this acceptable? My intention was simply to bring the branch up to date before adding my work.

It wasn't a problem. Since those exact same commits were already on main, Git simply removed those commits when I applied your PR to my local branch.

If there’s any problem with this approach, please let me know—and if so, I’d appreciate guidance on how to resolve it.

I suppose there are two ways to approach that.

If you see that changes to main since you created your branch won't have any effect on your work, you can simply avoid merging from main into your PR's branch.

Another option is to rebase on top of main. That will avoid inclusion of the new commits on top of your work, but it will require you to "force push" your changes to your PR's branch, which can sometimes cause issues with comments left on the previous state of your PR.

So, in the end, it depends. 😉

This is my first time contributing to this project, and I’m still getting comfortable with Git commands; apologies for missteps.

No reason to apologize. Git is a great tool, but it is challenging to master. TBH, I am not a Git expert either.

[neronsoda]

Thank you so much for your kind and detailed explanation.

I’m truly happy to contribute to junit :)

I’ll continue contributing whenever I can!👍