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

Added support for Kotest tests #243

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

ILikeYourHat
Copy link

Addresses #123

  • Added Kotest framework to project
  • Rewritten all tests in ktoml-file and ktoml-source modules
  • Rewritten a few tests in ktoml-core module
  • Sometimes making minor improvements in tests, like migrating away from TomlTable.fullTableName method or removing unused classes

Hope that you would like the way tests look now, and accept my humble contribution :)

@akuleshov7
Copy link
Owner

akuleshov7 commented Oct 18, 2023

Hey, thank you, you are awesome, we had big discussions about kotest #123
I will have a look and merge it

@ILikeYourHat
Copy link
Author

One more thing: I did this PR because I'm participating in the Hacktoberfest. I would be grateful if you add "hacktoberfest-accepted" label to this PR to make it count. But if for any reason you don't want to do it, I'm also ok with it 👍

@nulls nulls linked an issue Oct 19, 2023 that may be closed by this pull request
@akuleshov7
Copy link
Owner

One more thing: I did this PR because I'm participating in the Hacktoberfest. I would be grateful if you add "hacktoberfest-accepted" label to this PR to make it count. But if for any reason you don't want to do it, I'm also ok with it 👍

Added, am I right?

@ILikeYourHat
Copy link
Author

Added, am I right?

Yes, thank you!

@ILikeYourHat
Copy link
Author

Any update on this PR? Do you need anything more from me?


@ExperimentalSerializationApi
class PartialDecoderTest {
@Serializable
data class TwoTomlTables(val table1: Table1, val table2: Table2)
Copy link
Owner

Choose a reason for hiding this comment

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

why this was removed?

Copy link
Owner

Choose a reason for hiding this comment

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

I do not see this class anymore in the tests

Copy link
Author

Choose a reason for hiding this comment

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

It was removed because it was actually not used.

In this test you are providing a toml example which as a whole is parseable to TwoTomlTables. However, the thing that is being tested here is partial decoding. So you don't need to use the whole model, only the model of the part you are decoding. In other words, the TwoTomlTables model was used only to get the table2 value, which seems like a boilerplate to me

@@ -8,4 +8,5 @@ object Versions {
const val JUNIT = "5.7.1"
Copy link
Owner

Choose a reason for hiding this comment

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

Why you did not remove Junit then?

Copy link
Owner

Choose a reason for hiding this comment

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

Looks like Junit still remains @ILikeYourHat what was the idea?

Copy link
Author

Choose a reason for hiding this comment

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

Migrating to Kotest is a big change, and my idea was to migrate step by step, PR by PR, to make CR easier and avoid possible conflicts. However I can migrate all tests at once in this PR, if you prefer that way. It is up to you :)

Copy link
Owner

Choose a reason for hiding this comment

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

If we do a migration - it is difficult to keep both Unit and Kotest :(
Better to migrate everything, sorry 🙏

Copy link
Author

Choose a reason for hiding this comment

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

@akuleshov7 it took a while, but here you have it: everything migrated to Kotest :)

Please note that the actual runner for jvm is still JUnit5. Kotest also have an jvm test runner, but the test format is very different there: https://kotest.io/docs/framework/writing-tests.html. I know, the issue mentions also the change of the runner, but in my opinion:

  • the main benefit are assertions, and those are migrated
  • things like parametrized test, user friendly names of tests etc. can be achieved also in JUnit5 runner
  • this PR is big enough, and I don't have enough energy for increasing the scope

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.

Switch the testing framework to Kotest?
3 participants