first DAO test with h2 db setup #15
Conversation
build.gradle
Outdated
| @@ -16,9 +16,6 @@ repositories { | |||
| mavenCentral() | |||
| } | |||
|
|
|||
| if (JavaVersion.current() != JavaVersion.VERSION_1_8) { | |||
| throw new GradleException("This build must be run with Java 8") | |||
| } | |||
There was a problem hiding this comment.
do we need this? I have java 9
There was a problem hiding this comment.
Actually, we can remove. This check was more for guarding against compatibility changes and ensure we build with a stable version (but java 9 is fairly stable).
There was a problem hiding this comment.
Should we be using Java 9 with this project going forward?
There was a problem hiding this comment.
Sure. Java 9 is fairly stable anyways. Also comes with convenient factory methods for collections!
build.gradle
Outdated
| @@ -31,6 +28,7 @@ dependencies { | |||
| testCompile 'io.dropwizard:dropwizard-testing:1.3.5' | |||
| testCompile 'junit:junit:4.12' | |||
| testCompile 'org.mockito:mockito-core:2.19.0' | |||
| testCompile 'com.h2database:h2:1.4.197' | |||
There was a problem hiding this comment.
Ahh, overlooked testCompile. What are your thoughts on using testcontainers instead of H2?
There was a problem hiding this comment.
I was hoping that h2 would be compatible enough. I did not know about testcontainers, but as long as the setup is integrated in the test (with a Rule) then it looks great. We just need to verify it what dependencies are (docker installed etc). The fewer the better.
There was a problem hiding this comment.
There is a dependency on docker, which does add one more thing to get up and running. That said, for integration tests, that'd be fine and expected.
| import marquez.api.Owner; | ||
| import marquez.db.dao.fixtures.DAOSetup; | ||
|
|
||
| public class TestOwnerDAO { |
There was a problem hiding this comment.
Minor: I prefer starting test classes with the name of the class under test: <class>Test. For example, OwnerDAOTest
| public class TestOwnerDAO { | ||
|
|
||
| @ClassRule | ||
| public static final DAOSetup daoSetup = new DAOSetup(); |
There was a problem hiding this comment.
I'm wondering if we can simplify the app setup with:
@ClassRule
static final DropwizardAppRule<MarquezConfiguration> RULE =
new DropwizardAppRule<MarquezConfiguration>(
MarquezApplication.class,
ResourceHelpers.resourceFilePath("config.yaml"));source: http://dropwizard.readthedocs.io/en/stable/manual/testing.html
There was a problem hiding this comment.
If we decided to use testcontainers, the rule would be defined simply as:
@Rule
public PostgreSQLContainer postgres = new PostgreSQLContainer();source: SimplePostgreSQLTest
There was a problem hiding this comment.
The DAOSetup class is trying to simplify further the DropwizardAppRule, since we know the Config and App classes, have a config file etc. I started with the DARule.
|
|
||
| @Test | ||
| public void testUpdateOwner() { | ||
| ownerDAO.insert(new Owner("Amaranta")); |
There was a problem hiding this comment.
We should verify the result and/or assert that it was inserted.
There was a problem hiding this comment.
agreed :) The DAO doesn't have a "get" yet so I stopped there. It doesn't fail which mean that the db is setup properly. I wanted to have the DB setup to bootstrap DAO testing.
|
Is the idea that we merge this as the empty test suite and we can work on filling in with tests in future PRs? |
|
Yes, the idea was to bootstrap DAO test by setting up the DB |
|
Ok perfect. |
| return jdbi.onDemand(classT); | ||
| } | ||
| } | ||
|
|
* fix position of network graph and search bar, adjust padding for row with datasets * change legend position to fixed
* Add DropWizard Jackson Object Mapper * Add JobType * Add JsonUtils.newObjectMapper factory method * Add ModelGenerator.newIdWithBound helper function
* Add __init__.py for installation * Up the version
No description provided.