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

Reduce the need for @AutoConfigureTestDatabase(replace=NONE) when using a test-provided database #35253

Closed
wilkinsona opened this issue May 3, 2023 · 9 comments
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@wilkinsona
Copy link
Member

wilkinsona commented May 3, 2023

If a test class is using @ServiceConnection to create a test-specific DataSource, @DataJpaTest or any other sliced-test annotation that uses @AutoConfigureTestDatabase will replace it with an embedded database due to its default replacement policy of ANY. I believe the same problem will occur with @DynamicPropertySource so this problem isn't particularly new. That said we may not be able to fix it in maintenance releases, it'll depend on the nature and complexity of any fix. Assigning to 3.1.x for now.

@quaff
Copy link
Contributor

quaff commented May 4, 2023

That's #35125 trying to fix.

@wilkinsona
Copy link
Member Author

Unfortunately, I don't think #35125 is the right approach. As I said when closing the PR, I don't think the location of where connection details are defined is a strong enough signal to decide whether or not the database should be replaced. A user could have defined their own JdbcConnectionDetails that should be replaced. Alternatively, they could have used @DynamicPropertySource and then we'd want to keep the database defined by DataSourceAutoConfiguration.

@quaff
Copy link
Contributor

quaff commented Jan 31, 2024

@wilkinsona What about skip replacing DataSource if bean org.springframework.boot.testcontainers.service.connection.ServiceConnectionAutoConfiguration is present?

	if (registry.containsBeanDefinition("org.springframework.boot.testcontainers.service.connection.ServiceConnectionAutoConfiguration") ) {
		logger.info(
						"Skip replacing DataSource bean with embedded version due to @ServiceConnection is using");
		return;
	}

@wilkinsona
Copy link
Member Author

Thanks for the idea. Unfortunately, I don't think we can assume that the presence of ServiceConnectionAutoConfiguration guarantees that the replacement should not happen or that its absence guarantees that the replacement should happen. For example, as I said above, there's also the @DynamicPropertySource case where someone may not be using spring-boot-testcontainers but they still don't want any replacement to happen.

I think we need a general purpose way of having more control over the replacement, but I don't yet know what that looks like. I think anything that relies on specifics will be too brittle.

@wilkinsona wilkinsona modified the milestones: 3.1.x, 3.2.x May 20, 2024
@philwebb philwebb self-assigned this Jun 27, 2024
@philwebb
Copy link
Member

I have a possible solution to this at https://github.com/philwebb/spring-boot/tree/gh-35253, but it feels risky for a bug fix. Especially since adding @AutoConfigureTestDatabase(replace=Replace.NONE) is a pretty easy workaround.

I'm also not totally sure about how we should detect service connections. There's a FIXME in that code that I'd like to get opinions on from others in the team.

@philwebb philwebb added for: team-meeting An issue we'd like to discuss as a team to make progress type: enhancement A general enhancement and removed type: bug A general bug for: team-meeting An issue we'd like to discuss as a team to make progress labels Jun 28, 2024
@philwebb philwebb modified the milestones: 3.2.x, 3.4.x Jul 4, 2024
@philwebb philwebb changed the title @DataJpaTest and other slices annotated with @AutoConfigureTestDatabase replace a test-provided database by default Reduce the need for @AutoConfigureTestDatabase(replace=NONE) when using a test-provided database Jul 4, 2024
@philwebb philwebb modified the milestones: 3.4.x, 3.4.0-M3 Sep 12, 2024
@eddumelendez
Copy link
Contributor

eddumelendez commented Sep 12, 2024

@philwebb philwebb reopened this Sep 12, 2024
@philwebb
Copy link
Member

Thanks @eddumelendez, reopening to investigate :(

@philwebb
Copy link
Member

Thanks again for testing the SNAPSHOT @eddumelendez. I've pushed a fix if you want to try again.

@eddumelendez
Copy link
Contributor

eddumelendez commented Sep 12, 2024

I've tested locally and both tests are green now. Thanks @philwebb!

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

4 participants