Skip to content

Improve test coverage #732

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom
Merged
garydgregory merged 6 commits into from
Dec 12, 2025
Merged

Improve test coverage #732

garydgregory merged 6 commits into from
Dec 12, 2025
+142 −7

Conversation

@centic9
Copy link
Member

@centic9 centic9 commented Dec 11, 2025 •
edited
Loading

A few test-adjustments:

  • Enable one test which was disabled some time ago, but should work again now
  • Make sure the local is always "US" to not fail tests when run on systems which have set a different locale
  • Cover a few more lines/branches in some tests to slightly increase coverage
  • mark tests of deprecated methods with @deprecate to reduce warnings in the IDE

Thanks for your contribution to Apache Commons! Your help is appreciated!

Before you push a pull request, review this list:

  • Read the contribution guidelines for this project.
  • Read the ASF Generative Tooling Guidance if you use Artificial Intelligence (AI).
  • I used AI to create any part of, or all of, this pull request. Which AI tool was used to create this pull request, and to what extent did it contribute?
  • Run a successful build using the default Maven goal with mvn; that's mvn on the command line by itself.
  • Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible, but it is a best practice.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Each commit in the pull request should have a meaningful subject line and body. Note that a maintainer may squash commits during the merge process.

@centic9 centic9 force-pushed the add_more_test_coverage branch 4 times, most recently from ff46e31 to f9e5165 Compare December 11, 2025 20:04
@centic9
Cover a few lines/branches
f9e5165 
Add some more tests to cover a bit more.

Also apply some IDE-suggestions, assert-order,
simplify assertions.
garydgregory
garydgregory reviewed Dec 12, 2025
View reviewed changes
Copy link
Member

@garydgregory garydgregory left a comment

Choose a reason for hiding this comment

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

@centic9
Thank you for your PR. I can see that this improves the coverage in all except one case.
Please address all of my comments, and I will get this merged.

src/test/java/org/apache/commons/text/lookup/DateStringLookupTest.java Outdated
DateFormat.getInstance().parse(formatted); // throws ParseException
Locale prev = Locale.getDefault();
try {
// ensure that date-parser uses English/American presets
Copy link
Member

@garydgregory garydgregory Dec 11, 2025

Choose a reason for hiding this comment

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

Why is this important for this test?

That's not how you set with Locale's anyway, this component depends on JUnit Pioneer for this type of stuff (see @DefaultLocale)

I think you can remove this test as this class is already at 100% coverage:

Image

Copy link
Member Author

@centic9 centic9 Dec 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The test fails for me locally when the OS is set to "de" instead of "us".

I don't see @DefaultLocale anywhere, do you have a link?

Anyway, reverted for now as it actually does not increase coverage, but just fixes a local test failure for me.

Copy link
Member

@garydgregory garydgregory Dec 12, 2025

Choose a reason for hiding this comment

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

See https://junit-pioneer.org/docs/default-locale-timezone/

garydgregory
garydgregory approved these changes Dec 12, 2025
View reviewed changes
Copy link
Member

@garydgregory garydgregory left a comment

Choose a reason for hiding this comment

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

@centic9
Thank you for your updates.

@garydgregory garydgregory merged commit 9e34d3d into apache:master Dec 12, 2025
9 checks passed
@garydgregory garydgregory changed the title Add more test coverage Improve test coverage Dec 12, 2025
garydgregory added a commit that referenced this pull request Dec 12, 2025
@garydgregory
Bump the level of test coverage checks.
94200bc 
See: Improve test coverage #732.
@garydgregory
Copy link
Member

garydgregory commented Dec 12, 2025

Thank you @centic9
After this PR, I was able to raise the bar on our coverage checks to:

    <commons.jacoco.haltOnFailure>true</commons.jacoco.haltOnFailure>
    <commons.jacoco.classRatio>1.00</commons.jacoco.classRatio>
    <commons.jacoco.instructionRatio>0.99</commons.jacoco.instructionRatio>
    <commons.jacoco.methodRatio>0.99</commons.jacoco.methodRatio>
    <commons.jacoco.branchRatio>0.95</commons.jacoco.branchRatio>
    <commons.jacoco.lineRatio>0.99</commons.jacoco.lineRatio>    
    <commons.jacoco.complexityRatio>0.97</commons.jacoco.complexityRatio>

@centic9 centic9 deleted the add_more_test_coverage branch December 12, 2025 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@garydgregory garydgregory garydgregory approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@centic9 @garydgregory