Move tests from jvmTest to commonTest

[burnoo]

I've decided to follow up work done in #14 by moving tests from jvmTest to commonMain (as suggested by @ccjernigan here). It would be the first step for making the library multiplatform.

PR details

• Removed Moshi and JSON parsing from tests and replaced it with hardcoded list
• Replaced Java API functions with pure Kotlin (String.format() and Character.digit())
• Moved tests from jvmTest to commonTest
• Updated Gradle dependency locks

Next steps

1. Move library API and code that does not depend on the Java API from jvmMain to commonMain (using actual/expect)
2. Add additional platforms one-by-one

Extra thoughts

1. Have you thought about replacing dependabot with renovate? It has support for Gradle catalog's .toml files, so it would automate updating Gradle dependencies.
2. I tried to update library's Gradle locks by using command suggested in the docs (./gradlew dependencies --write-locks). It didn't work for me - I had to use module specific Gradle task: ./gradlew bip39-lib:dependencies --write-locks. It would be great to either include it in the docs or make bip39-lib:dependencies task run while running dependencies.

Author

• Self-review: Did you review your own code in GitHub's web interface? Code often looks different when reviewing the diff in a browser, making it easier to spot potential bugs.
• Automated tests: Did you add appropriate automated tests for any code changes?
• Code coverage: Did you check the code coverage report for the automated tests? _While we are not looking for perfect coverage, the tool can point out potential cases that have been missed. Run ./gradlew check then coverage reports are generated under build/reports/kover.
• Documentation: Did you update documentation as appropriate? (e.g README.md, etc.)
• Rebase and squash: Did you pull in the latest changes from the main branch and squash your commits before assigning a reviewer? Having your code up to date and squashed will make it easier for others to review. Use best judgement when squashing commits, as some changes (such as refactoring) might be easier to review as a separate commit.

Reviewer

• Checklist review: Did you go through the code with the Code Review Guidelines checklist?
• Ad hoc review: Did you perform an ad hoc review? In addition to a first pass using the code review guidelines, do a second pass using your best judgement and experience which may identify additional questions or comments. Research shows that code review is most effective when done in multiple passes, where reviewers look for different things through each pass.
• Automated tests: Did you review the automated tests?
• Manual tests: Did you review the manual tests?
• How is code coverage affected by this PR? We encourage you to compare coverage before and after changes and when possible, leaving it in a better place.
• Documentation: Did you review Docs and README.md as appropriate?

[ccjernigan]

1. Have you thought about replacing dependabot with renovate? It has support for Gradle catalog's .toml files, so it would automate updating Gradle dependencies.

Great question. Renovate is on our radar. I have a personal preference to avoid giving a third party write access to our Git repo, given the security breaches that have occurred in the past few years due to GitHub apps (e.g. Travis and Heroku). I keep hoping that Dependabot will get better Gradle support, especially now that Gradle supports toml version catalogs. But it may be worth seriously evaluating the tradeoffs.

2. I tried to update library's Gradle locks by using command suggested in the docs (./gradlew dependencies --write-locks). It didn't work for me - I had to use module specific Gradle task: ./gradlew bip39-lib:dependencies --write-locks. It would be great to either include it in the docs or make bip39-lib:dependencies task run while running dependencies.

This looks like a gap in the documentation, thanks for noting it. I filed an issue. I note that the docs you point to don't include the resolveAll task in our issue template: https://github.com/zcash/kotlin-bip39/blob/main/.github/ISSUE_TEMPLATE/dependency.md

[ccjernigan]

Thank you for this PR. Having this Multiplatform work split up into several smaller PRs helps us get parts of it merged much more quickly.

I have one small inline comment, although I don't think it is necessary to address. I will wait a bit of time to merge until after I hear from you, just to confirm you agree with my assessment.

[ccjernigan]

Because the Kotlin common implementation is more complex and harder to follow compared to the version relying on the JVM, one very small suggestion would be considering whether we need a test case for the to/from hex methods. They're doing some non-trivial work, and our test assertions rely on them.

One test case could be a round-trip through toHex and fromHex, asserting the data comes out the same. Another could be converting bytes to hex and making sure the output matches a known expected value.

That said, if there was a bug here our tests should fail due to how our tests are set up. So I think we have parity with our JVM tests from before.

Put another way: if you were writing this from scratch I'd say we need a test case. But because it worked before under the JVM, we can be more confident that the conversion here is valid.

[burnoo]

For me at first it felt not quite natural to test code created just for testing. But you're right - this is important part of testing logic and there is no reason why it shouldn't be tested. Added in 1c0fa82

[burnoo]

I have a personal preference to avoid giving a third party write access to our Git repo, given the security breaches that have occurred in the past few years due to GitHub apps (e.g. Travis and Heroku).

Renovate has another GitHub app called "Forking Renovate" (docs), which does not require write access to code (permissions table). It opens PRs from a fork, similarly to any other contributor. I think it's a good security improvement.

I keep hoping that Dependabot will get better Gradle support, especially now that Gradle supports toml version catalogs.

I've already lost my hope, GitHub issue with over 200 thumbs up is open for over a year and a half. Also Renovate is much more configurable, and has a extra nice features, such as Dependency Dashboard.

[ccjernigan]

Looks great. There's just one minor ktlint issue blocking CI. Do you want to push a fix for that, then I can merge this?

[ccjernigan]

@burnoo Thank you very much for this great enhancement. It is a pleasure working with you, and I appreciate the effort that went into this contribution. These small pieces are really helpful to move us towards a multiplatform future!