Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Improve enforcer rule error messages #343

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Improve enforcer rule error messages #343

wants to merge 8 commits into from

Conversation

elharo
Copy link
Contributor

@elharo elharo commented Jan 9, 2025 •
edited
Loading

  1. Avoid abbreviations
  2. Increase test coverage
  3. Make messages platform independent
  4. Don't double print paths to files
  5. Verify test setup with assumptions rather than asserts

@elharo elharo changed the title Make exception messages platform independent Improve enforcer rule error messages Jan 9, 2025
@elharo elharo marked this pull request as ready for review January 12, 2025 14:16
slawekjaranowski
slawekjaranowski reviewed Jan 13, 2025
View reviewed changes
enforcer-rules/src/main/java/org/apache/maven/enforcer/rules/files/AbstractRequireFiles.java
buf.append(message + System.lineSeparator());
buf.append(message + '\n');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how it will be worked on windows console ....
Can anybody confirm ...
@michael-o @Bukama

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, this isn't right

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this method does not print anything on the console. It throws an exception object that should be identical regardless of the platform that creates it. Where this message goes is outside the scope of this class. It might be written to a log file and never show up on a console. It might be suppressed completely.

In 2025 Windows shells behave in the obvious way if you send them a linefeed, so this works just fine across platforms if someone does happen to print this string to a console. Maybe System.lineSeparator was needed in Windows 95. It isn't now.

michael-o
michael-o requested changes Jan 13, 2025
View reviewed changes
enforcer-rules/src/main/java/org/apache/maven/enforcer/rules/files/AbstractRequireFiles.java
buf.append(message + System.lineSeparator());
buf.append(message + '\n');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, this isn't right

@Bukama
Copy link
Contributor

Bukama commented Jan 13, 2025

Build (mvn clean verify site) of this branch breaks (on Windows 10): Please have a look.

[INFO] Reactor Summary for Apache Maven Enforcer 3.6.0-SNAPSHOT:
[INFO]
[INFO] Apache Maven Enforcer ................................................................................ SUCCESS [ 28.782 s]
[INFO] Apache Maven Enforcer API ............................................................................ SUCCESS [  4.314 s]
[INFO] Apache Maven Enforcer Built-In Rules ................................................................. SUCCESS [ 13.093 s]
[INFO] Apache Maven Enforcer Plugin ......................................................................... FAILURE [ 10.733 s]
[INFO] Apache Maven Enforcer Extension ...................................................................... SKIPPED
[INFO] --------------------------------------------------------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] --------------------------------------------------------------------------------------------------------------------------
[INFO] Total time:  57.264 s
[INFO] Finished at: 2025-01-13T17:27:56+01:00
[INFO] --------------------------------------------------------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-site-plugin:3.12.1:site (default-site) on project maven-enforcer-plugin: Error generating maven-plugin-report-plugin:3.13.1:report report: Cannot invoke "org.codehaus.plexus.configuration.PlexusConfiguration.getChild(String)" because "this.configuration" is null -> [Help 1]

>mvn --version
Apache Maven 4.0.0-rc-2 (273314404f85ec3c089e295d8b4e0cb18c287cf5)
Maven home: C:\apache-maven-4.0.0-rc-2
Java version: 21.0.1, vendor: Eclipse Adoptium, runtime: C:\JDK\Java21
Default locale: de_DE, platform encoding: UTF-8
OS name: "windows 10", version: "10.0", arch: "amd64", family: "windows"```

Bukama
Bukama reviewed Jan 13, 2025
View reviewed changes
...cer-rules/src/test/java/org/apache/maven/enforcer/rules/files/TestRequireFilesDontExist.java Outdated
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;
import static org.junit.jupiter.api.Assertions.*;
Copy link
Contributor

@Bukama Bukama Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the introduction of star imports on purpose? (here and in other test files)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should was be catched by checkstyle ... 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apache/maven-parent#228

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, not intentional. Let me fix it.

@slawekjaranowski
Copy link
Member

@Bukama site is broken for all maven-plugins and Maven 4.x
https://issues.apache.org/jira/browse/MPLUGIN-502

slawekjaranowski
slawekjaranowski reviewed Feb 1, 2025
View reviewed changes
enforcer-rules/src/test/java/org/apache/maven/enforcer/rules/files/TestRequireFilesSize.java
Comment on lines -119 to +125
assertFalse(f.exists());
assumeFalse(f.exists());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will change test logic ... at all
When file will be not deleted .. test will be marked as skipped not failed

Is it intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we don't want to test that the JDK's delete method works as expected, and if the code under test isn't even executed then the test has been skipped, not failed. This case inspired me to write https://cafe.elharo.com/testing/assume-vs-assert/

elharo
elharo commented Feb 1, 2025
View reviewed changes
Copy link
Contributor Author

@elharo elharo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PTAL

@elharo
Copy link
Contributor Author

elharo commented Feb 20, 2025

Ping

# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@slawekjaranowski slawekjaranowski slawekjaranowski left review comments

@Bukama Bukama Bukama left review comments

@michael-o michael-o michael-o requested changes

@khmarbaise khmarbaise Awaiting requested review from khmarbaise

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@elharo @Bukama @slawekjaranowski @michael-o