Skip to content

Make String to Boolean parameter conversion more strict#3178

Merged
marcphilipp merged 6 commits intojunit-team:mainfrom
s-rwe:issues/3177
Apr 28, 2023
Merged

Make String to Boolean parameter conversion more strict#3178
marcphilipp merged 6 commits intojunit-team:mainfrom
s-rwe:issues/3177

Conversation

@s-rwe
Copy link
Copy Markdown
Contributor

@s-rwe s-rwe commented Feb 27, 2023 •
edited by marcphilipp
Loading

Cause use of anything other than 'true' or 'false' (case-insensitive) fail in conversion, to make potential typos explicitly visible (instead of implicitly just treating all else as 'false')

Includes a few tests for failures on invalid values, for conversions handled by StringToBooleanAndCharPrimitiveConverter.

Issue: #3177

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

Definition of Done

@s-rwe
Force using 'true' or 'false' for boolean args
e560035 
Cause use of anything other than 'true' or 'false' (case-insensitive)
fail in conversion, to make potential typos explicitly visible (instead
of implicitly just treating all else as 'false')

Includes a few tests for failures on invalid values, for conversions
handled by StringToBooleanAndCharPrimitiveConverter.

Issue: #3177
@s-rwe s-rwe mentioned this pull request Feb 27, 2023
Closed
sbrannen
sbrannen requested changes Feb 27, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@sbrannen sbrannen left a comment

Choose a reason for hiding this comment

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

Thanks for submitting the PR so quickly!

I requested a few minor changes.

Comment thread ...upiter-params/src/main/java/org/junit/jupiter/params/converter/DefaultArgumentConverter.java
Comment thread ...r-params/src/test/java/org/junit/jupiter/params/converter/DefaultArgumentConverterTests.java Outdated
Comment thread ...r-params/src/test/java/org/junit/jupiter/params/converter/DefaultArgumentConverterTests.java
@sbrannen sbrannen self-assigned this Feb 27, 2023
@s-rwe
Copy link
Copy Markdown
Contributor Author

s-rwe commented Feb 27, 2023

Thanks for submitting the PR so quickly!

I requested a few minor changes.

Sure thing, likewise thanks for the quick review :)

Requested changes pushed accordingly.

sbrannen
sbrannen requested changes Feb 27, 2023
View reviewed changes
Comment thread ...upiter-params/src/main/java/org/junit/jupiter/params/converter/DefaultArgumentConverter.java Outdated
Comment thread ...r-params/src/test/java/org/junit/jupiter/params/converter/DefaultArgumentConverterTests.java Outdated
sbrannen
sbrannen approved these changes Feb 27, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@sbrannen sbrannen left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The proposal looks good to me from a technical standpoint.

All that's missing is an entry in the release notes and, potentially, a note in the user guide.

Though I think we should hold off on those last two tasks until the rest of the team has voiced their approval.

@sbrannen sbrannen linked an issue Feb 27, 2023 that may be closed by this pull request
Implicit boolean conversion in parameterized tests should be strict and accept only true or false #3177
Closed
@marcphilipp
Copy link
Copy Markdown
Member

marcphilipp commented Mar 3, 2023

Team Decision: Make it more strict by default since it's a more sane behavior.

@marcphilipp
Copy link
Copy Markdown
Member

marcphilipp commented Mar 3, 2023

@s-rwe Could you please add an entry to the release notes and update the table in the User Guide?

@s-rwe
Copy link
Copy Markdown
Contributor Author

s-rwe commented Mar 3, 2023

@s-rwe Could you please add an entry to the release notes and update the table in the User Guide?

Ok sure thanks, done accordingly.

marcphilipp
marcphilipp reviewed Apr 28, 2023
View reviewed changes
Comment thread documentation/src/docs/asciidoc/release-notes/release-notes-5.10.0-M1.adoc Outdated
@marcphilipp marcphilipp assigned marcphilipp and unassigned sbrannen Apr 28, 2023
@marcphilipp
Copy link
Copy Markdown
Member

marcphilipp commented Apr 28, 2023

I'll take it from here.

@marcphilipp marcphilipp changed the title Force using 'true' or 'false' for boolean args Make String to Boolean parameter conversion more strict Apr 28, 2023
@marcphilipp marcphilipp merged commit f79e38b into junit-team:main Apr 28, 2023
@marcphilipp
Copy link
Copy Markdown
Member

marcphilipp commented Apr 28, 2023

@s-rwe Thank you for you contribution! 🙇‍♂️

@s-rwe s-rwe deleted the issues/3177 branch May 17, 2023 13:39
@fuqua79 fuqua79 mentioned this pull request Jul 26, 2023
Closed
1 task
@Treverix Treverix mentioned this pull request Oct 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sbrannen sbrannen sbrannen approved these changes

@marcphilipp marcphilipp marcphilipp approved these changes

Assignees

@marcphilipp marcphilipp

Labels

component: Jupiter theme: parameterized tests theme: programming model type: enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Implicit boolean conversion in parameterized tests should be strict and accept only true or false

3 participants

@s-rwe @marcphilipp @sbrannen