-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add example with dao tests #1
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.
Nice PR @tonilopezmr congrats! 🎉 🎉 I've added some comments I'd like to review with you before merging this PR 😃
README.md
Outdated
1. Clone or download in your machine [kotlin-plugin][ktlint-plugin] | ||
2. Run `sbt scripted`. This will release locally the `1.0.9-SNAPSHOT` version in your local `ivy cache`. | ||
3. Add the kotlin plugin in `project/plugins.sbt` with the given version. `addSbtPlugin("com.hanhuy.sbt" % "kotlin-plugin" % "1.0.9-SNAPSHOT")` | ||
|
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.
Why don't you use the snapshots repository? https://www.scala-sbt.org/1.x/docs/Resolvers.html
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.
That version isn't be released, then you need to publish locally. I'm going to explain it in the README 👍
app/controllers/Application.kt
Outdated
} | ||
} | ||
|
||
fun processError(error: Throwable): Result = badRequest() |
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.
You should not return bad request if you catch an exception. You can return a 5xx error instead.
app/controllers/Application.kt
Outdated
|
||
fun processError(error: Throwable): Result = badRequest() | ||
fun created(developer: Developer): Result = created(developer.toJson()) | ||
fun ok(developer: Developer): Result = ok(developer.toJson()) |
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.
Every function in this file but the controller methods associated to the routes should be private.
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.
Yes that's right
private const val KARUMI_EMAIL = "@karumi.com" | ||
} | ||
|
||
fun isKarumiDeveloper(developer: Developer): Boolean = |
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.
As this is a single function class you'll never mock I'd implement this as an object with a public method or a an invoke operator.
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.
I will never 👍
private val developerValidator: KarumiDeveloperValidator | ||
) { | ||
|
||
operator fun invoke(developer: Developer): Developer? = |
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.
Nice usage of the invoke operator 😃
fun givenANewKarumiDev(): NewDeveloperJson = NewDeveloperJson( | ||
username = "Unknown", | ||
email = "email@karumi.com" | ||
) |
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.
I'd move these methods to a mother object file.
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.
Yes, I will use a technique we use in our internal projects, an interface with the given methods with a default implementation to use kotlin delegates 👍
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.
I have used the kotlin delegates to provide the objects in this commit to fix this comment, We use in our projects to create stubs of our objects with mockito and add it to the injector.
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.
Nice!
test/controllers/ApplicationTest.kt
Outdated
.bodyJson(newDeveloper.toJson())) | ||
|
||
val createdDeveloper = result.asObject(Developer::class) | ||
val developer = Developer(createdDeveloper.id, newDeveloper.username, newDeveloper.email) |
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.
Do you really need this line? Can't you use newDeveloper
?
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.
The POST json body is:
{
"username": "unkown",
"email@karumi.com"
}
Then doesn't have any id
field, if I compare a new developer to the developer just I have created then they are going to be different because the new developer doesn't have the id
.
Reviewing the test, I think I can assert the username
and the email
of the createdDeveloper
with the newDeveloper
and assert the obtainedDeveloper
got by id with the createdDeveloper
(which these two has the same id).
What do you think? I'm going to write it and push it.
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.
I have changed it 47895ed
test/controllers/ApplicationTest.kt
Outdated
|
||
data class InvalidJson(val invalid: String = "") | ||
fun getById(id: UUID) = dao.getById(id) | ||
fun create(developer: Developer) = dao.create(developer) |
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 should be private.
test/controllers/ApplicationTest.kt
Outdated
@Test | ||
fun `developer POST should returns 400 if the json is bad formed`() { | ||
val result = route(app, fakeRequest("POST", "/developer") | ||
.bodyJson(InvalidJson().toJson())) |
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 not a bad formed JSON body. This is actually a valid JSON body. I guess you mean you are going to send a request using a not expected JSON body.
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.
yes it is, I was wrong
test/developers/DeveloperDaoTest.kt
Outdated
import org.junit.Test | ||
import utils.ApplicationWithDatabase | ||
|
||
class DeveloperDaoTest : ApplicationWithDatabase() { |
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.
If you are not mocking the dao as part of the previous controller tests, aren't you testing the same twice?
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.
The only difference I see is (apart from the scope of the tests) is that here you test the update process too.
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.
Yes, that's right. I started to write this tests then I moved to write the application test, I can remove the create developer and get by id developer tests.
I want to keep this test file here to show how to test only the DAO part.
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.
Thanks @pedrovgs I have changed your suggestions |
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.
👍 ready to merge @tonilopezmr Really nice PR 👏
📝 Notes
The new version of kotlin-plugin for sbt it's not released then I need to use a local release snapshotRight now has been released the 1.0.9 version.1.0.9-SNAPSHOT
.🎩 What is the goal?
📝 How is it being implemented?
Download the code of kotlin-plugin for sbt and runJust add the kotlin-plugin with the 1.0.9 version.sbt scripted
to add in the last version1.0.9-SNAPSHOT
to the localivy cache
, after that, we can run tests in kotlin with sbt and use kotlin in play framework.GET /developer/:developerId
endpoint to get a developer by idPOST /developer
endpoint to create a new karumi developer given the json body:It's a valid karumi developer if it has an karumi email.
postgresql v9.6
in production, to run in local you need to run the following docker command:docker-compose up -d
.H2
database to run the tests, to reset the database I useApplicationWithDatabase
parent class in the tests which want to use the H2 database.💥 How can it be tested?
sbt test