Skip to content

Polish tests #42725

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
Closed
wants to merge 1 commit into from
Closed

Polish tests #42725

wants to merge 1 commit into from

Conversation

quaff
Copy link
Contributor

@quaff quaff commented Oct 17, 2024

  1. Improve null safety
  2. Replace lambda with method reference

@quaff
Polishing tests
cad7ef2 
1. Improve null safety
2. Replace lambda with method reference
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 17, 2024
@mhalbritter mhalbritter added type: task A general task and removed status: waiting-for-triage An issue we've not yet triaged labels Oct 17, 2024
@mhalbritter mhalbritter added this to the 3.2.x milestone Oct 17, 2024
@mhalbritter mhalbritter changed the title Polishing tests Polish tests Oct 17, 2024
wilkinsona
wilkinsona requested changes Oct 17, 2024
View reviewed changes
.../test/java/org/springframework/boot/context/properties/ConfigurationPropertiesBeanTests.java
@@ -197,6 +197,7 @@ void getWhenHasFactoryMethodAndBeanAnnotationFavorsFactoryMethod() throws Throwa
void getWhenHasValidatedBeanBindsWithBeanAnnotation() throws Throwable {
get(ValidatedBeanConfiguration.class, "validatedBean", (propertiesBean) -> {
Validated validated = propertiesBean.asBindTarget().getAnnotation(Validated.class);
assertThat(validated).isNotNull();
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is an improvement. It makes the test more verbose and I think the benefit is negligible. Should validated be null, the NPE that would occur on the next line when calling validated.value() will make that clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It makes IDEA and code analysis tools happy, is it worthy?

Copy link
Member

Choose a reason for hiding this comment

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

No, I don't think so.

.../test/java/org/springframework/boot/context/properties/ConfigurationPropertiesBeanTests.java
@@ -205,6 +206,7 @@ void getWhenHasValidatedBeanBindsWithBeanAnnotation() throws Throwable {
void getWhenHasValidatedFactoryMethodBindsWithFactoryMethodAnnotation() throws Throwable {
get(ValidatedMethodConfiguration.class, "annotatedBean", (propertiesBean) -> {
Validated validated = propertiesBean.asBindTarget().getAnnotation(Validated.class);
assertThat(validated).isNotNull();
Copy link
Member

Choose a reason for hiding this comment

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

As above. Please revert.

.../test/java/org/springframework/boot/context/properties/ConfigurationPropertiesBeanTests.java
@@ -213,6 +215,7 @@ void getWhenHasValidatedFactoryMethodBindsWithFactoryMethodAnnotation() throws T
void getWhenHasValidatedBeanAndFactoryMethodBindsWithFactoryMethodAnnotation() throws Throwable {
get(ValidatedMethodAndBeanConfiguration.class, "validatedBean", (propertiesBean) -> {
Validated validated = propertiesBean.asBindTarget().getAnnotation(Validated.class);
assertThat(validated).isNotNull();
Copy link
Member

Choose a reason for hiding this comment

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

As above. Please revert.

...a/org/springframework/boot/context/properties/ConfigurationPropertiesBindExceptionTests.java
@@ -36,6 +36,7 @@ void createFromBeanHasDetails() {
ApplicationContext applicationContext = new AnnotationConfigApplicationContext(Example.class);
ConfigurationPropertiesBean bean = ConfigurationPropertiesBean.get(applicationContext,
applicationContext.getBean(Example.class), "example");
assertThat(bean).isNotNull();
Copy link
Member

Choose a reason for hiding this comment

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

As above. Please revert. Additionally, getBean(Class) will never return null, throwing a NoSuchBeanDefinitionException instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ConfigurationPropertiesBean.get() is @Nullable

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I misread the diff. Regardless, the same reasoning as above still applies. I don't think null checks such as this in tests are beneficial.

.../src/test/java/org/springframework/boot/context/properties/ConfigurationPropertiesTests.java
@@ -1197,6 +1197,7 @@ void loadWhenBoundToRandomPropertyPlaceholder() {
void boundPropertiesShouldBeRecorded() {
load(NestedConfiguration.class, "name=foo", "nested.name=bar");
BoundConfigurationProperties bound = BoundConfigurationProperties.get(this.context);
assertThat(bound).isNotNull();
Copy link
Member

Choose a reason for hiding this comment

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

As above. Please revert.

@wilkinsona wilkinsona modified the milestones: 3.2.x, 3.4.x Oct 18, 2024
@wilkinsona wilkinsona added the for: merge-with-amendments Needs some changes when we merge label Oct 18, 2024
@wilkinsona wilkinsona modified the milestones: 3.4.x, 3.4.0-RC1 Oct 18, 2024
wilkinsona pushed a commit that referenced this pull request Oct 18, 2024
@quaff @wilkinsona
Polish tests
906ebb6 
Replace lambdas with method references

See gh-42725
@wilkinsona
Copy link
Member

Thanks, @quaff.

I've merged this without the null checks. We can consider this topic again as and when we tackle #10712, building on the Framework team's experience where they treat main and test source differently.

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

@wilkinsona wilkinsona wilkinsona requested changes

Assignees
No one assigned
Labels
for: merge-with-amendments Needs some changes when we merge type: task A general task
Projects
None yet
Milestone
3.4.0-RC1
Development

Successfully merging this pull request may close these issues.
4 participants
@quaff @wilkinsona @spring-projects-issues @mhalbritter