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 setAllowBeanDefinitionOverriding method to ApplicationContextRunner #18019

Closed
philwebb opened this issue Aug 30, 2019 · 6 comments
Closed
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@philwebb
Copy link
Member

See #17963 for background. As well as fixing the name generation we should add a setAllowBeanDefinitionOverriding method and align with SpringApplication.

@philwebb philwebb added the type: enhancement A general enhancement label Aug 30, 2019
@philwebb philwebb added this to the 2.2.x milestone Aug 30, 2019
@snicoll snicoll self-assigned this Aug 30, 2019
@snicoll snicoll modified the milestones: 2.2.x, 2.3.x Oct 16, 2019
snicoll added a commit to snicoll/spring-boot that referenced this issue Dec 30, 2019
For consistency with SpringApplication, this commit disables bean
overriding by default in ApplicationContextRunner. Bean overriding can
be enabled again using withAllowBeanDefinitionOverriding.

Closes spring-projectsgh-18019
@snicoll
Copy link
Member

snicoll commented Dec 30, 2019

I've given this one a try and a few tests in spring-boot-autoconfigure started failing. They all involve @ComponentScan one way or the other and it looks like the component scan directive is processed twice. I haven't investigated more than that but it can be reproduced using https://github.com/snicoll/spring-boot/tree/gh-18019

@snicoll
Copy link
Member

snicoll commented Dec 30, 2019

I've fixed a few tests that were a little bit brittle but I can't seem to grasp what CassandraReactiveDataAutoConfigurationTests is doing. Flagging for team attention to see if someone on the team can put me out of my misery.

@snicoll snicoll added the for: team-attention An issue we'd like other members of the team to review label Dec 30, 2019
@wilkinsona
Copy link
Member

What was the problem with CassandraReactiveDataAutoConfigurationTests? It doesn't seem to be using the application context runner at the moment.

@snicoll
Copy link
Member

snicoll commented Jan 6, 2020

Sorry I meant CassandraReactiveRepositoriesAutoConfigurationTests

@wilkinsona
Copy link
Member

wilkinsona commented Jan 6, 2020

It looks a bit broken to me. The excludeFilter doesn't appear to do anything. Component scanning itself seems to allow the test to pass when a local Cassandra instance isn't available. It does that by picking up the mocked Session from the TestConfiguration inner-class. That's quite a nasty side-effect. It must be picking up plenty of other stuff too that's unintentional and makes the tests rather confusing.

There's a similar pattern across a number of test classes related to Cassandra. I suspect it may have been written once and then copied to other classes as they were added. It looks to me as if we should rework all of the affected classes to remove the use of @ComponentScan entirely.

snicoll added a commit that referenced this issue Jan 8, 2020
@snicoll
Copy link
Member

snicoll commented Jan 8, 2020

Thanks Andy, I've polished things up in bc066d2

@snicoll snicoll removed the for: team-attention An issue we'd like other members of the team to review label Jan 8, 2020
snicoll added a commit to snicoll/spring-boot that referenced this issue Jan 17, 2020
For consistency with SpringApplication, this commit disables bean
overriding by default in ApplicationContextRunner. Bean overriding can
be enabled again using withAllowBeanDefinitionOverriding.

Closes spring-projectsgh-18019
@snicoll snicoll modified the milestones: 2.3.x, 2.3.0.M2 Jan 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants