-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Provide a Testcontainer for Neo4j. #993
Conversation
There was a problem hiding this 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
modules/neo4j/src/main/java/org/testcontainers/containers/Neo4jContainer.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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!
modules/neo4j/src/test/java/org/testcontainers/containers/Neo4jContainerTest.java
Outdated
Show resolved
Hide resolved
e9697f6
to
72ba7ff
Compare
Thanks a lot for your detailed feedback everyone. I have used the provided mechanism for accepting licenses (only necessary for our enterprise license). 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. |
I don't understand why CI fails now… |
I can't see a reason for that CI failure, but have re-triggered... |
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! |
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. |
Co-Authored-By: michael-simons <michael@simons.ac>
…r-license-acceptance.txt to .gitignore. This allows the user or another developer to just copy our example into place.
3dafbd0
to
1ba19b0
Compare
Done. |
The test failure has nothing to do with this PR - I'll tackle that with #1008. Please ignore! |
There was a problem hiding this 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!
Fantastic! Thanks for accepting my contribution. |
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:
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.