Provide a Testcontainer for Neo4j. by michael-simons · Pull Request #993 · testcontainers/testcontainers-java · GitHub
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.

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

Provide a Testcontainer for Neo4j. #993

Merged
merged 11 commits into from
Dec 8, 2018
Merged

Provide a Testcontainer for Neo4j. #993

merged 11 commits into from
Dec 8, 2018

Conversation

michael-simons
Copy link
Contributor

I'd like to propose a Testcontainer for Neo4j, an open source graph database as in this pull request.

The test container is build with our official Docker image and provides the following features to the user:

  • To retrieve a Bolt url for usage with one of the official drivers, most likely the Java driver
  • To retrieve HTTP and HTTPS urls for the transactional REST api
  • Especially to disable authentication or chose a password
  • Also explicitly use the enterprise version of Neo4j

While we offer a test harness and users may opt to use an embedded version of our database during test, it seems that many people think it's a better practice to test any database access against "the real thing".

The container would be useful to us and allegedly to some other projects using Neo4j as well.

We have been using a container very similar while spiking our efforts of a reactive client for Neo4j.

kiview
kiview requested changes Nov 26, 2018
View reviewed changes
Copy link
Member

@kiview kiview left a comment

Choose a reason for hiding this comment

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

Hey @michael-simons,
great PR and cool to see, that you are already using Testcontainers.

Just some small remarks, I'll give it a proper review later.

modules/neo4j/src/main/java/org/testcontainers/containers/Neo4jContainer.java Outdated Show resolved Hide resolved
modules/neo4j/src/main/java/org/testcontainers/containers/Neo4jContainer.java Outdated Show resolved Hide resolved
modules/neo4j/src/main/java/org/testcontainers/containers/Neo4jContainer.java Outdated Show resolved Hide resolved
...s/neo4j/src/test/java/org/testcontainers/containers/Neo4jContainerEnterpriseEditionTest.java Outdated Show resolved Hide resolved
@testcontainers testcontainers deleted a comment Nov 27, 2018
@testcontainers testcontainers deleted a comment Nov 27, 2018
rnorth
rnorth requested changes Dec 1, 2018
View reviewed changes
Copy link
Member

@rnorth rnorth left a comment

Choose a reason for hiding this comment

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

This is a really nice PR, and a great addition to what is available for developers. Thank you.

I just have a couple of queries (pretty minor), but in general I think we'll be happy to accept this soon.
Cheers!

docs/usage/neo4j_container.md Outdated Show resolved Hide resolved
docs/usage/neo4j_container.md Outdated Show resolved Hide resolved
modules/neo4j/src/test/java/org/testcontainers/containers/Neo4jContainerTest.java Outdated Show resolved Hide resolved
@michael-simons
Copy link
Contributor Author

Thanks a lot for your detailed feedback everyone.

I have used the provided mechanism for accepting licenses (only necessary for our enterprise license).
I have kept the original test under an assumption, disabled as long as the user doesn't explicit add the license acceptance file.

I added AssertJ to check for exceptions in tests, also rewrote the older ones.

Note that I rebased onto your master, so you don't have to deal with merges.

@michael-simons
Copy link
Contributor Author

I don't understand why CI fails now…

@rnorth
Copy link
Member

rnorth commented Dec 3, 2018

I can't see a reason for that CI failure, but have re-triggered...

@rnorth
Copy link
Member

rnorth commented Dec 3, 2018

Bizarrely it looks like we've suddenly been hit with this issue (or something like it): gradle/gradle#3318

This is obviously not a problem with the PR itself, so I'll have to investigate!

@rnorth
Copy link
Member

rnorth commented Dec 4, 2018

Please could you rebase onto master/merge? I think we have fixed a strange problem on Travis CI in #1004, so this should resolve the failing build.

@michael-simons
Copy link
Contributor Author

Done.

@michael-simons michael-simons mentioned this pull request Dec 5, 2018
Closed
@testcontainers testcontainers deleted a comment Dec 8, 2018
@testcontainers testcontainers deleted a comment Dec 8, 2018
@rnorth
Copy link
Member

rnorth commented Dec 8, 2018

The test failure has nothing to do with this PR - I'll tackle that with #1008. Please ignore!

@rnorth rnorth added this to the next milestone Dec 8, 2018
rnorth
rnorth approved these changes Dec 8, 2018
View reviewed changes
Copy link
Member

@rnorth rnorth left a comment

Choose a reason for hiding this comment

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

This looks good to me now. Thank you for the contribution @michael-simons!

@rnorth rnorth merged commit e435dd4 into testcontainers:master Dec 8, 2018
@michael-simons
Copy link
Contributor Author

Fantastic! Thanks for accepting my contribution.

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

@kiview kiview kiview requested changes

@rnorth rnorth rnorth approved these changes

@bsideup bsideup Awaiting requested review from bsideup

Assignees
No one assigned
Labels
type/feature
Projects
None yet
Milestone
1.10.3
Development

Successfully merging this pull request may close these issues.
3 participants
@michael-simons @rnorth @kiview