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.

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

Introduce tests for MultiValueMap #25160

Closed

Conversation

midumitrescu
Copy link
Contributor

imho improved tests for gh-25140, as solicited by @jhoeller

As promised in gh-25159, I am submitting a second suggestion for Testing the exiting code of

CollectionUtils.toMultiValueMap

The suggestion was to create for this MultiValueMap same tests as for

LinkedMultiValueMap

present in

LinkedMultiValueMapTests

This, however, produced a very high code duplication, which I did not find very elegant.

I think the better way is to use Jupiter properly, to instantiate various types of MultiValueMap and test the contract for each of them.

Like this test code is not duplicated and the contracts are consistent with each other.

This is just a suggestion.

I am open for suggestions in case this does not fit your expectations.

Kind regards

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label May 28, 2020
Copy link
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.

I think the use of parameterized tests is a good idea.

I've requested a few minor changes and will review again once those have been made.

@sbrannen sbrannen added in: core Issues in core modules (aop, beans, core, context, expression) status: waiting-for-feedback We need additional information before we can continue type: task A general task and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels May 29, 2020
@sbrannen sbrannen changed the title Improved Tests for gh-25140 Introduce tests for MultiValueMap May 29, 2020
@midumitrescu
Copy link
Contributor Author

@sbrannen thank you very much for your review.

I will also change the name of the test to:

MultiValueMapAdaptersTests to reflect current changes done by Juergen.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels May 29, 2020
@midumitrescu
Copy link
Contributor Author

midumitrescu commented May 29, 2020

@sbrannen done

I did not trust myself to delete

org.springframework.util.LinkedMultiValueMapTests

But imho they are now fully migrated with the parameterized tests

@snicoll snicoll removed the status: feedback-provided Feedback has been provided label Aug 25, 2023
@snicoll snicoll self-assigned this Aug 25, 2023
@snicoll snicoll added this to the 6.1.0-RC1 milestone Aug 25, 2023
snicoll pushed a commit that referenced this pull request Aug 25, 2023
snicoll added a commit that referenced this pull request Aug 25, 2023
@snicoll snicoll closed this in 9b3f456 Aug 25, 2023
@snicoll
Copy link
Member

snicoll commented Aug 25, 2023

Thanks @midumitrescu, I've enabled the tests that were disabled as the fix has been added in the meantime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: task A general task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants