Skip to content

Consider removing in-memory database configuration from slice tests #42360

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
vpavic opened this issue Sep 18, 2024 · 4 comments
Closed

Consider removing in-memory database configuration from slice tests #42360

vpavic opened this issue Sep 18, 2024 · 4 comments
Labels
status: declined A suggestion or change that we don't feel we should currently apply

Comments

@vpavic
Copy link
Contributor

vpavic commented Sep 18, 2024

To begin with, I'm aware of recent improvements made by #35253.

However, I'd still ask to consider removing in-memory database configuration from slice tests where it's used (@JdbcTest, @DataJpaTest, @DataJdbcTest) for the following reasons:

  • integration testing data access layer against an in-memory isn't really a best practice (and Spring Boot should guide developers towards best practices)
  • most major RDBMSs are available in containerized form (and Testcontainers is a obviously well established solution that helps there)
  • the aforementioned Reduce the need for @AutoConfigureTestDatabase(replace=NONE) when using a test-provided database #35253 IMO actually makes it easier to change the default as it should lower the impact
  • changing the default will align all relational database slices in this regard (@JooqTest doesn't configure an in-memory database)

In practice, I haven't worked on a project where data access layer was primarily tested using an in-memory database probably since ~2016 so that's a lot of occurrences of @AutoConfigureTestDatabase(replace=NONE).

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 18, 2024
@quaff
Copy link
Contributor

quaff commented Sep 19, 2024

I don't think so, at least @DataJpaTest should keep it, since most test cases are testing logic of persistence not sql, using which database is not important.
Testcontainers is great but it requires docker, unit tests shouldn't rely on external services, people should create dedicate integration test suite to use it.

@wilkinsona
Copy link
Member

Thanks for the suggestion but I don't think we can do this. In addition to @quaff's points above, we also know from experience that a significant portion of Spring Boot's user base cannot or does not use Docker for various reasons.

@JooqTest doesn't configure an in-memory database

Thanks for spotting this. It's not moving things in the direction that you'd like, but I think we should correct this by adding @AutoConfigureTestDatabase to @JooqTest. I've opened #42371 to do that.

@wilkinsona wilkinsona closed this as not planned Won't fix, can't repro, duplicate, stale Sep 19, 2024
@wilkinsona wilkinsona added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 19, 2024
@vpavic
Copy link
Contributor Author

vpavic commented Sep 19, 2024

In practice, I don't remember seeing a real world project that didn't use at least some custom SQL (or JPQL) statements. Using JPA/Hibernate also doesn't mean developers shouldn't leverage native (often vendor specific) capabilities of their RDMBS - quite a few Hibernate experts are actually trying to raise awareness of that. Finally, testing against an in-memory database makes it easier to hide issues caused by change of behavior when upgrading to a new Hibernate release.

we also know from experience that a significant portion of Spring Boot's user base cannot or does not use Docker for various reasons.

That's fine, but shouldn't be a reason for Spring Boot to have something that's clearly not a best practice as a default. Also with the change they'd still be just an annotation away from opting in to auto-configured in-memory database. That's also more explicit and less magic, which is good thing from my point of view.

Like I said, I've seen really a lot of @AutoConfigureTestDatabase(replace=NONE) over the years that leads me to believe a better default is to have developers opt in and be specific regarding something like this, rather than having to constantly opt out.

I must say, when it comes to data access support, Spring Boot has some quite frustrating defaults (see OSIV/OEMIV).

@wilkinsona wilkinsona added the for: team-meeting An issue we'd like to discuss as a team to make progress label Sep 19, 2024
@wilkinsona
Copy link
Member

wilkinsona commented Sep 19, 2024

I'm not sure we have sufficient evidence to state, categorically as you have done above, that what Boot is doing here is "clearly not a best practice as a default". Reducing things to being a best practice or not being a best practice often strips away a lot of context from different people's situations and the different variables that have to be weighed up.

If you're in a position to use Docker (and with some of their recent changes that probably means being in a position to pay for it), using a containerized database for integration tests could be the best choice given the situation. If you're not in a position to use Docker, then obviously you can't do that. Given a choice between using an in-memory database pretending to be your actual database and requiring everyone on a team and CI workers to have the actual database set up and running, using an in-memory database could be the best choice given the situation.

With the changes in Boot 3.4, I think we're doing a pretty good job of catering for both of the situations described above. If you write an integration test using @DataJpaTest or the like and you've chosen to have an in-memory database on the classpath, we'll make use of it for the test. Equally, if you write such an integration test and you've chosen to use Testcontainers in your test, we'll now make use of it automatically without having to disable the database replacement.

to have developers opt in and be specific regarding something like this

Boot doesn't add an in-memory database to the classpath by default. The user has to opt in to using an in-memory database for testing by adding one to the classpath.

rather than having to constantly opt out.

Haven't we addressed this in Boot 3.4? If there are still cases where @AutoConfigureTestDatabase(replace=NONE) is needed, I would prefer that we see if we can address those rather than making things worse for those who have chosen to use an in-memory database.

@philwebb philwebb removed the for: team-meeting An issue we'd like to discuss as a team to make progress label Oct 2, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

No branches or pull requests

5 participants