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.

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

UpdaterDao Unittests #21

Closed
wants to merge 1 commit into from
Closed

UpdaterDao Unittests #21

wants to merge 1 commit into from

Conversation

katsadim
Copy link

@katsadim katsadim commented Oct 22, 2018

What's this PR for?

Unit tests for UpdaterDao.
This PR is the second part of resolving issue: #10

What to emphasize on when reviewing?

  • Now the DB connection is injected into UpdaterDao for easier mocking. No need to use fancy tools to mess around with the bytecode cause of the static method call DbConnEngine.getConnection().
  • I took the liberty of introducing Spock framework into the project. Spock will certainly make unit testing easier and fun for sure.
  • This will fail for now because there is a need to accept the Linked Pull Request.

Linked Pull Requests

Repo: k0r0pt/common_build_config#8

Copy link
Member

@sudiptosarkar sudiptosarkar left a comment

Choose a reason for hiding this comment

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

@katsadim Not sure if Groovy may be the right choice for this. Even if we do keep Groovy tests in there, we need a way for it to add to the coverage. Checkout the build logs for this PR. Your test class didn't run at all... Please resolve this so that gradle test runs the groovy tests as well...

Other than that, nicely written... 👍

@katsadim
Copy link
Author

katsadim commented Oct 24, 2018

@sudiptosarkar, trust me Spock is one of the best framework for writing unit tests. Groovy makes the unit tests much more readable and an absolute beauty. I say that you should give it a try.

The groovy gradle plugin should be applied first in order to run the test. If you accept the linked PR k0r0pt/common_build_config#8 then the spock tests will be enabled.

image

@sumitsarkar
Copy link
Collaborator

sumitsarkar commented Oct 25, 2018

@katsadim I agree that Spock indeed is a very good framework for BDD. But the question is whether introducing a different JVM language just for the sake of introducing BDD makes sense for this project.

I don't see the RoI in introducing a new language and paradigm when the complexity of the project is not very big.

As for what you are trying to test... Since SQLite requires a single lock to write to the file system, you'd see that the method for insertion of row is synchronized. Taking your first idea further, we can simply inject the UpdaterDao into the scrapper implementations. That does exactly what you are looking for.
As for testing UpdaterDao, the purpose is to test the generated PreparedStatement. The easy way would be to extract the logic of the preparation of the statement to a separate function and write simple JUnit tests for validating it.

Having said that, I took your idea and created #23. You can have a look at that.

@katsadim
Copy link
Author

Imho, introducing a different language is not such a big deal, if it indeed makes testing easier and more enjoyable. Junit compared to Spock is a pain to write and leads to code repetition and really hard to understand tests. As a side note I have seen smaller projects using Spock!

I am glad you liked the dependency injection idea and you took a step even further.

Just a suggestion here would be to not refer to the concrete class when passing it around but rather declare an interface. This way, when we want for example to save a station in a different storage (DB, cloud, microservice, you name it), then the switch would be easy.

@sumitsarkar
Copy link
Collaborator

Just a suggestion here would be to not refer to the concrete class when passing it around but rather declare an interface. This way, when we want for example to save a station in a different storage (DB, cloud, microservice, you name it), then the switch would be easy.

Agreed! We can introduce the interface when we have varying implementations for storage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants