-
Notifications
You must be signed in to change notification settings - Fork 25
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
Conversation
Hey, thank you, you are awesome, we had big discussions about kotest #123 |
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? |
Yes, thank you! |
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) |
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 this was removed?
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 do not see this class anymore in the tests
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.
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" |
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 you did not remove Junit then?
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.
Looks like Junit still remains @ILikeYourHat what was the idea?
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.
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 :)
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 we do a migration - it is difficult to keep both Unit and Kotest :(
Better to migrate everything, sorry 🙏
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.
@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
Addresses #123
ktoml-file
andktoml-source
modulesktoml-core
moduleTomlTable.fullTableName
method or removing unused classesHope that you would like the way tests look now, and accept my humble contribution :)