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

Add tempest testing #33

Merged
merged 2 commits into from
Mar 8, 2021
Merged

Add tempest testing #33

merged 2 commits into from
Mar 8, 2021

Conversation

zhxnlai
Copy link
Collaborator

@zhxnlai zhxnlai commented Feb 16, 2021

This PR adds two ways to test interaction with DynamoDB

  • tempest-testing-docker
  • tempest-testing-jvm

with support for

  • tempest-testing-junit4
  • tempest-testing-junit5

This PR also removes the dependency on misk-testing and misk-dynamodb-testing

@zhxnlai zhxnlai requested review from swankjesse and tyiu February 16, 2021 04:57
zhxnlai
zhxnlai commented Feb 16, 2021
View reviewed changes
tempest-testing/src/main/kotlin/app/cash/tempest/testing/TestDynamoDbClient.kt
import com.google.common.util.concurrent.Service
import kotlin.reflect.KClass

interface TestDynamoDbClient : Service {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

New API

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks great!

zhxnlai
zhxnlai commented Feb 16, 2021
View reviewed changes
tempest-testing/src/main/kotlin/app/cash/tempest/testing/TestDynamoDbServer.kt
/**
* A DynamoDB test server running in-process or in a local Docker container.
*/
interface TestDynamoDbServer : Service {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

New API

zhxnlai
zhxnlai commented Feb 16, 2021
View reviewed changes
tempest-testing/src/main/kotlin/app/cash/tempest/testing/TestTable.kt
* Use [configureTable] to customize the table creation request for testing, such as to configure
* the secondary indexes required by `ProjectionType.ALL`.
*/
data class TestTable internal constructor(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

New API

zhxnlai
zhxnlai commented Feb 16, 2021
View reviewed changes
tempest2-testing-docker/src/main/kotlin/app/cash/tempest2/testing/DockerDynamoDbServer.kt Outdated
import software.amazon.awssdk.services.dynamodb.model.DeleteTableRequest
import software.amazon.awssdk.services.dynamodb.model.DynamoDbException

object DockerDynamoDbServer : AbstractIdleService(), TestDynamoDbServer {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

New API

zhxnlai
zhxnlai commented Feb 16, 2021
View reviewed changes
tempest-testing-jvm/src/main/kotlin/app/cash/tempest/testing/JvmDynamoDbServer.kt Outdated
import com.google.common.util.concurrent.AbstractIdleService
import java.io.File

object JvmDynamoDbServer : AbstractIdleService(), TestDynamoDbServer {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

New API

zhxnlai
zhxnlai commented Feb 16, 2021
View reviewed changes
tempest/src/test/kotlin/app/cash/tempest/musiclibrary/TestUtils.kt Outdated
import com.amazonaws.services.dynamodbv2.model.Projection
import com.amazonaws.services.dynamodbv2.model.ProjectionType

fun testDb() = TestDynamoDb.Builder(JvmDynamoDbServer)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

API usage

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks awesome.

zhxnlai
zhxnlai commented Feb 16, 2021
View reviewed changes
tempest/src/test/kotlin/app/cash/tempest/DynamoDbQueryableTest.kt
val module = MusicDbTestModule()
@RegisterExtension
@JvmField
val db = testDb()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

junit5 API usage

zhxnlai
zhxnlai commented Feb 16, 2021
View reviewed changes
dependencies.gradle
@@ -20,9 +24,6 @@ ext.dep = [
"ktlintVersion": "0.40.0",
"loggingApi": "io.github.microutils:kotlin-logging:1.7.9",
"mavenPublishGradlePlugin": "com.vanniktech:gradle-maven-publish-plugin:0.12.0",
"miskAws2DynamodbTesting": "com.squareup.misk:misk-aws2-dynamodb-testing:0.17.0-20210210.0438-e43f047",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removing misk dependencies

Copy link
Contributor

Choose a reason for hiding this comment

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

Yay!

swankjesse
swankjesse approved these changes Mar 1, 2021
View reviewed changes
Copy link
Contributor

@swankjesse swankjesse left a comment

Choose a reason for hiding this comment

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

Looks very very good.

I’d like for you to land this as-is, and then optionally address any feedback in follow-ups. I hope to be a lot faster on the turnaround for those.

tempest/src/test/java/app/cash/tempest/interop/InteropTestUtils.java

public class InteropTestUtils {

public static TestDynamoDb testDb() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This API looks great.

tempest/src/test/kotlin/app/cash/tempest/musiclibrary/TestUtils.kt Outdated
import com.amazonaws.services.dynamodbv2.model.Projection
import com.amazonaws.services.dynamodbv2.model.ProjectionType

fun testDb() = TestDynamoDb.Builder(JvmDynamoDbServer)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks awesome.

tempest/src/test/kotlin/app/cash/tempest/interop/JavaInteropTest.kt

@Inject lateinit var aliasDb: AliasDb
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needing Guice in these tests is nice for the library.

tempest/src/test/kotlin/app/cash/tempest/DynamoDbQueryableTest.kt
private val musicTable get() = musicDb.music
private val reservedWordsTable get() = reservedWordsDb.table
private val musicTable by lazy { db.logicalDb<MusicDb>().music }
private val reservedWordsTable by lazy { db.logicalDb<ReservedWordsDb>().table }
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if there’s a way to make these lazy internally rather than in the public API. For testing it could initialize on demand or assert if it isn’t or something.

Just to save boilerplate in the tests themselves!

dependencies.gradle
@@ -20,9 +24,6 @@ ext.dep = [
"ktlintVersion": "0.40.0",
"loggingApi": "io.github.microutils:kotlin-logging:1.7.9",
"mavenPublishGradlePlugin": "com.vanniktech:gradle-maven-publish-plugin:0.12.0",
"miskAws2DynamodbTesting": "com.squareup.misk:misk-aws2-dynamodb-testing:0.17.0-20210210.0438-e43f047",
Copy link
Contributor

Choose a reason for hiding this comment

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

Yay!

tempest-testing-internal/src/main/kotlin/app/cash/tempest/testing/DefaultTestDynamoDbClient.kt Outdated
import com.amazonaws.services.dynamodbv2.AmazonDynamoDBStreams
import com.google.common.util.concurrent.AbstractIdleService

class DefaultTestDynamoDbClient(
Copy link
Contributor

Choose a reason for hiding this comment

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

Documentation?

Copy link
Contributor

Choose a reason for hiding this comment

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

In particular, what’s the difference between this and the TestDynamoDbClient base class?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TestDynamoDbClient is just the interface?

tempest-testing-internal/src/main/kotlin/app/cash/tempest/testing/Logger.kt Outdated Show resolved Hide resolved
tempest-testing-internal/src/main/kotlin/app/cash/tempest/testing/TestUtils.kt Outdated
import com.amazonaws.services.dynamodbv2.model.ProvisionedThroughput

object TestUtils {
val port: Int = pickPort()
Copy link
Contributor

Choose a reason for hiding this comment

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

I don’t like the stateful port on this object. Is there an object the caller could use instead?

tempest-testing-internal/src/main/kotlin/app/cash/tempest/testing/TestUtils.kt Outdated Show resolved Hide resolved
tempest-testing-junit4/src/main/kotlin/app/cash/tempest/testing/TestDynamoDb.kt
import org.junit.rules.ExternalResource
import java.util.concurrent.ConcurrentHashMap

class TestDynamoDb private constructor(
Copy link
Contributor

Choose a reason for hiding this comment

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

Document that it stays running?

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if you could get away with a single JUnit4 and JUnit5 extension by using only Guava service as the public API?

There’s nothing particularly Dynamo2 or Dynamo1 specific in this, just the types it imports.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We do need 2 different TestTable classes tho to allow developers to create GSIs in SDK 1 and 2

@zhxnlai zhxnlai merged commit 1ebdb4e into master Mar 8, 2021
@zhxnlai zhxnlai deleted the zl-210205-tempest-testing branch March 8, 2021 03:08
@zhxnlai zhxnlai mentioned this pull request Mar 8, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swankjesse swankjesse swankjesse approved these changes

@tyiu tyiu Awaiting requested review from tyiu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@zhxnlai @swankjesse