Skip to content

Simplify FromXmlRuleSetTest#testFactoryIgnoreCreateRule by removing try/catch/fail - #292

Draft
siddiqueirshad wants to merge 1 commit into
apache:masterfrom
siddiqueirshad:fix-commons-digester-issues
Draft

siddiqueirshad wants to merge 1 commit into
apache:masterfrom
siddiqueirshad:fix-commons-digester-issues

Conversation

@siddiqueirshad

Copy link
Copy Markdown

Modernize exception verification in \FromXmlRuleSetTest#testFactoryIgnoreCreateRule\ to use JUnit 5 \�ssertDoesNotThrow\ consistent with \�ssertThrows\ in the same test class, and close missing parenthesis and correct grammar in \src/changes/changes.xml.

} catch (final Exception e) {
fail("This exception should have been ignored: " + e.getClass().getName());
}
assertDoesNotThrow( () -> newLoader( createRules( rules ) ).newDigester().parse( new StringReader( xml ) ),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't use assertDoesNotThrow, it's just clutter IMO. Why would you even do that on the last and only assert in a test?

@garydgregory
garydgregory marked this pull request as draft November 2, 2026 13:29
@siddiqueirshad
siddiqueirshad force-pushed the fix-commons-digester-issues branch from 59616cf to 21233b1 Compare November 2, 2026 15:23
@siddiqueirshad siddiqueirshad changed the title Use assertDoesNotThrow in FromXmlRuleSetTest and fix syntax in changes.xml Simplify FromXmlRuleSetTest#testFactoryIgnoreCreateRule by removing try/catch/fail Nov 2, 2026
@siddiqueirshad

Copy link
Copy Markdown
Author

Hi @garydgregory, thank you for the feedback! You're completely right. I have removed �ssertDoesNotThrow and simply let the test method execute directly without the redundant try/catch/fail block, and reverted the changes to changes.xml.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants