Conversation
# Conflicts: # transact/src/test/java/dev/dbos/transact/workflow/WorkflowMgmtTest.java
|
FYI, I'm OOF at PASS this week and for Thanksgiving next week. This looks cool, but not sure when I'll have a chance to review. |
|
FYI, this looks good but I haven't made it work w/ my local environment (I'm running docker inside of WSL). Working on fixing this on my end - I can't merge approve this until I can confirm it works for me locally. |
|
Also note, the cli tests were not updated to use testcontainers. I will look at that after I update my environment to work w/ testcontainers. |
transact/build.gradle.kts
Outdated
| testImplementation("io.rest-assured:rest-assured:5.4.0") | ||
| testImplementation("io.rest-assured:json-path:5.4.0") | ||
| testImplementation("io.rest-assured:xml-path:5.4.0") | ||
| testImplementation("org.testcontainers:postgresql:1.21.3") |
There was a problem hiding this comment.
As per https://java.testcontainers.org/modules/databases/postgres/, this appears to be an outdated version
Resolved all merge conflicts by keeping testcontainers setup: - All tests now extend DbSetupTestBase and use testcontainers - Updated build.gradle.kts to include both testcontainers and maven-artifact dependencies - Added dataSource field to DbSetupTestBase for tests that need direct database access - Converted 6 new test files to use DbSetupTestBase: * MetricsTest * SingleExecutionTest * PatchTest * PartitionedQueuesTest * ForkTest * GarbageCollectionTest
|
FYI, I'm working on some db connection changes. Might make sense to give your updates a day or two |
# Conflicts: # transact/build.gradle.kts # transact/src/test/java/dev/dbos/transact/client/ClientTest.java # transact/src/test/java/dev/dbos/transact/config/ConfigTest.java # transact/src/test/java/dev/dbos/transact/database/MetricsTest.java # transact/src/test/java/dev/dbos/transact/database/SystemDatabaseTest.java # transact/src/test/java/dev/dbos/transact/execution/DBOSExecutorTest.java # transact/src/test/java/dev/dbos/transact/execution/LifecycleTest.java # transact/src/test/java/dev/dbos/transact/execution/RecoveryServiceTest.java # transact/src/test/java/dev/dbos/transact/execution/SingleExecutionTest.java # transact/src/test/java/dev/dbos/transact/invocation/CustomSchemaTest.java # transact/src/test/java/dev/dbos/transact/invocation/DirectInvocationTest.java # transact/src/test/java/dev/dbos/transact/invocation/MultiClassInstanceTest.java # transact/src/test/java/dev/dbos/transact/invocation/StartWorkflowTest.java # transact/src/test/java/dev/dbos/transact/issues/Issue218.java # transact/src/test/java/dev/dbos/transact/migrations/BackCompatTest.java # transact/src/test/java/dev/dbos/transact/migrations/MigrationManagerTest.java # transact/src/test/java/dev/dbos/transact/notifications/EventsTest.java # transact/src/test/java/dev/dbos/transact/notifications/NotificationServiceTest.java # transact/src/test/java/dev/dbos/transact/queue/PartitionedQueuesTest.java # transact/src/test/java/dev/dbos/transact/queue/QueuesTest.java # transact/src/test/java/dev/dbos/transact/scheduled/SchedulerServiceTest.java # transact/src/test/java/dev/dbos/transact/step/StepsTest.java # transact/src/test/java/dev/dbos/transact/workflow/AsyncWorkflowTest.java # transact/src/test/java/dev/dbos/transact/workflow/ForkTest.java # transact/src/test/java/dev/dbos/transact/workflow/GarbageCollectionTest.java # transact/src/test/java/dev/dbos/transact/workflow/QueueChildWorkflowTest.java # transact/src/test/java/dev/dbos/transact/workflow/SyncWorkflowTest.java # transact/src/test/java/dev/dbos/transact/workflow/TimeoutTest.java # transact/src/test/java/dev/dbos/transact/workflow/UnifiedProxyTest.java # transact/src/test/java/dev/dbos/transact/workflow/WorkflowMgmtTest.java
|
Hey @devhawk, |
|
Looks good. I'm not typically a fan of inheritance - but in this case it makes sense since it sets up the I am going to hold off on merging this for a day or two. We are considering some restructuring / project refactoring that will likely require test refactoring as well. I'm a big fan of testcontainers, so we're definitely going to leverage your work. I just want to see how the other work stream is going to shape up. |
| .withDatabaseUrl(postgres.getJdbcUrl()) | ||
| .withDbUser(postgres.getUsername()) | ||
| .withDbPassword(postgres.getPassword()); | ||
| dataSource = SystemDatabase.createDataSource(dbosConfig); |
There was a problem hiding this comment.
It looks like we're always creating the data source but never closing it
| protected static void onetimeSetup() throws Exception { | ||
| postgres.start(); | ||
| dbosConfig = | ||
| DBOSConfig.defaults("systemdbtest") |
There was a problem hiding this comment.
nitpick: default app name should be something like transact-java-test
|
FYI, we're moving to a purely instance based API for DBOS (#306), which is going to affect almost every test. I'm going to start working on what that looks like in the tests before deciding if I want to merge this PR first or not. I don't want to make you do extra work, but if I'm making drastic changes to the test suite, there's not much point in merging this PR only to change everything again. In particular, we may want to move to us a PG container per test instead of per test class, so we can run tests in parallel. We will get to using test containers and you will be included among our contributors either way. Thanks for your patience on this @hannosgit! |
2 participants
Add testcontainers as test dependency for the setup of a Postgresql instance for testing. This allows running the tests on a local machine. The only requirement is Docker.
This PR also moves the setup of the DB connection into a common base class.