Skip to content
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

Replace try-catch constructs in tests with assertThrows #215

Merged
merged 3 commits into from
Jan 11, 2025
Merged

Replace try-catch constructs in tests with assertThrows #215

merged 3 commits into from
Jan 11, 2025

Conversation

strangelookingnerd
Copy link
Contributor

As requested in #214 (comment)

  • Simplified tests by replacing try-catch constructs with assertThrows and assertDoesNotThrow

Verified with mvn clean verify that build passes before and after my changes.
Overall number of tests and coverage did not change.

raboof
raboof reviewed Jan 9, 2025
View reviewed changes
Copy link
Member

@raboof raboof left a comment

Choose a reason for hiding this comment

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

Overall these seem like very nice improvements, thanks!

I added some minor questions - I'm not too familiar with the codebase yet so these are not necessarily criticisms of the PR, but more for my understanding ;)

src/test/java/org/apache/commons/jxpath/issues/JXPath113Test.java Show resolved Hide resolved
src/test/java/org/apache/commons/jxpath/issues/JXPath113Test.java Outdated Show resolved Hide resolved
src/test/java/org/apache/commons/jxpath/issues/JXPath172Test.java Show resolved Hide resolved
src/test/java/org/apache/commons/jxpath/ri/compiler/VariableTest.java Show resolved Hide resolved
raboof
raboof approved these changes Jan 9, 2025
View reviewed changes
Copy link
Member

@raboof raboof left a comment

Choose a reason for hiding this comment

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

Nice, LGTM! There's one failing GHA job, but that one's marked 'experimental', I think that means we don't expect it to pass.

garydgregory
garydgregory requested changes Jan 10, 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.

Hello @strangelookingnerd
Thank you for updating the PR. I have the same comments as before though, see embedded comments.

src/test/java/org/apache/commons/jxpath/ri/JXPathContextReferenceImplTestCase.java Outdated Show resolved Hide resolved
src/test/java/org/apache/commons/jxpath/ri/StressTest.java Outdated Show resolved Hide resolved
src/test/java/org/apache/commons/jxpath/ri/StressTest.java Outdated Show resolved Hide resolved
src/test/java/org/apache/commons/jxpath/util/ClassLoaderUtilTest.java Outdated Show resolved Hide resolved
src/test/java/org/apache/commons/jxpath/util/ClassLoaderUtilTest.java Outdated Show resolved Hide resolved
src/test/java/org/apache/commons/jxpath/ri/model/beans/BadlyImplementedFactoryTest.java Show resolved Hide resolved
@garydgregory garydgregory changed the title Replace try-catch constructs in tests with assertThrows and assertDoesNotThrow Replace try-catch constructs in tests with assertThrows Jan 11, 2025
@garydgregory garydgregory merged commit 7a3b96f into apache:master Jan 11, 2025
17 checks passed
garydgregory added a commit that referenced this pull request Jan 11, 2025
@garydgregory
Replace try-catch constructs in tests with assertThrows #215
2136c5b
@strangelookingnerd strangelookingnerd deleted the improve_assertions branch January 11, 2025 09:59
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

@raboof raboof raboof 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.
3 participants
@strangelookingnerd @raboof @garydgregory