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

Add tests for ControllerAdviceBean#resolveBean() and revise existing tests #33401

Conversation

tafjwr
Copy link
Contributor

@tafjwr tafjwr commented Aug 18, 2024

Overview

I think ControllerAdviceBean#resolveBean() should be tested, because it is a public API and depends on the cache system, so I'd like to add tests for that.

Related Issues

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Aug 18, 2024
@tafjwr tafjwr force-pushed the add-controller-advice-bean-resove-bean-tests branch from 8867941 to 3b7fa19 Compare August 18, 2024 03:58
@tafjwr tafjwr force-pushed the add-controller-advice-bean-resove-bean-tests branch from 3b7fa19 to 328b721 Compare August 18, 2024 05:11
@tafjwr
Copy link
Contributor Author

tafjwr commented Aug 18, 2024

Plus, I noticed that the some test method pairs perform precisely the same tests (xxxForBeanName and xxxForBeanInstance). There is no longer a ControllerAdviceBean constructor that accepts a bean instance, so xxxForBeanInstance test methods could be omitted.

@Test
void orderedWithLowestPrecedenceByDefaultForBeanName() {
assertOrder(SimpleControllerAdvice.class, Ordered.LOWEST_PRECEDENCE);
}
@Test
void orderedWithLowestPrecedenceByDefaultForBeanInstance() {
assertOrder(SimpleControllerAdvice.class, Ordered.LOWEST_PRECEDENCE);
}

@sbrannen sbrannen added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Aug 18, 2024
@sbrannen
Copy link
Member

sbrannen commented Aug 18, 2024

Plus, I noticed that the some test method pairs perform precisely the same tests (xxxForBeanName and xxxForBeanInstance). There is no longer a ControllerAdviceBean constructor that accepts a bean instance, so xxxForBeanInstance test methods could be omitted.

Good catch.

Please remove the obsolete "bean instance" tests.

@sbrannen sbrannen added 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 Aug 18, 2024
@sbrannen sbrannen added this to the 6.2.0-RC1 milestone Aug 18, 2024
@sbrannen sbrannen self-assigned this Aug 18, 2024
@tafjwr
Copy link
Contributor Author

tafjwr commented Aug 18, 2024

Thank for your advice! I removed the obsolete tests on 8d045ad

@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 Aug 18, 2024
@sbrannen sbrannen changed the title Add ControllerAdviceBean#resolveBean() tests Add tests for ControllerAdviceBean#resolveBean() and revise existing tests Aug 18, 2024
@sbrannen sbrannen closed this in 4ffeddb Aug 18, 2024
sbrannen added a commit that referenced this pull request Aug 18, 2024
@sbrannen sbrannen removed the status: feedback-provided Feedback has been provided label Aug 18, 2024
@sbrannen
Copy link
Member

This has been merged into main in 4ffeddb and revised in 346b6f7.

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: task A general task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants