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

Load jdbc driver with current classloader instead of system #261

Merged
merged 1 commit into from
Jan 16, 2017

Conversation

vkrot
Copy link
Contributor

@vkrot vkrot commented Jan 11, 2017

Load jdbc driver with current classloader instead of system classloader. When running tests in SBT system classloader does not contain jdbc driver - SBT uses separate URLClassloader.

Maybe there are some reasons to use system classloader instead of current one, that I'm not aware of? With this fix tests run well in SBT with scalatest.

…ed. When running tests in SBT system classloader does not contain jdbc driver - SBT uses separate URLClassloader
@rnorth
Copy link
Member

rnorth commented Jan 14, 2017

@vkrot thanks for this - I'll do a little more reading to try and double check the implications of moving this to the current classloader. I'm perfectly OK with this in principle, though.
Thanks!

@bsideup bsideup requested a review from rnorth January 15, 2017 16:18
@bsideup
Copy link
Member

bsideup commented Jan 15, 2017

According to http://www.kfu.com/~nsayer/Java/dyn-jdbc.html

"But why not use something like URLClassLoader and the overload of class.forName() that lets you specify the ClassLoader?" Because the DriverManager will refuse to use a driver not loaded by the system ClassLoader.

But since we're about about the tests, it's not an issue, because we don't actually load it dynamically.

So LGTM from me.

@bsideup bsideup added this to the 1.1.8 milestone Jan 15, 2017
@rnorth
Copy link
Member

rnorth commented Jan 16, 2017

@bsideup thanks - that's an interesting fact to note! This looks good to me too, so I'll merge.

Side note: I think I may have done something similar in a fork when trying to (ab)use testcontainers to support this technique for jOOQ class generation. I remember there were some definite classloading tweaks required, but I'm afraid I don't have the details to hand. As I'd like to resurrect this Flyway-Testcontainers-jOOQ experiment one day soon I might have to tweak this again, but this time would do so aware of the needs of SBT.

@vkrot if some time (no rush) you could point me at a minimal SBT+Testcontainers example repo, it would be useful to be able to refer back to for future regression testing.

@rnorth rnorth merged commit c740e17 into testcontainers:master Jan 16, 2017
@vkrot
Copy link
Contributor Author

vkrot commented Jan 27, 2017

@rnorth , I'm really sorry for long delay, just forgot about this. Here is the minimal sbt project with testcontainers postgresql module - https://github.com/vkrot/testcontainers-sbt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants