-
Notifications
You must be signed in to change notification settings - Fork 37
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
Unit tests db schemas hashes #3016
Conversation
@@ -69,8 +69,10 @@ import org.wordpress.android.fluxc.persistence.migrations.MIGRATION_7_8 | |||
import org.wordpress.android.fluxc.persistence.migrations.MIGRATION_8_9 | |||
import org.wordpress.android.fluxc.persistence.migrations.MIGRATION_9_10 | |||
|
|||
const val WC_DATABASE_VERSION = 36 |
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 moved the DB version to a constant for easier reference in unit tests.
|
||
@Test | ||
fun testDBHashes() { | ||
for(i in 3..WC_DATABASE_VERSION){ |
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.
We will iterate from every DB version starting from 3 (first DB schema) and ending in the last version
import org.junit.runner.RunWith | ||
|
||
@RunWith(AndroidJUnit4::class) | ||
class MigrationSchemasTests { |
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 put that into MigrationTests
, just to remind people that they actually don't test validity of migration here and should write a test similar to all the rest tests there
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.
Fixed here
} | ||
|
||
@Test | ||
fun testDBHashes() { |
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.
np: shell we at least try to name tests someone readable, like we do in Woo repo?
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.
Using spaces is not allowed because of the DEX version, I updated the name of the test following the same pattern as the rest of the test in MigrationTests
com.android.tools.r8.internal.Tb: Space characters in SimpleName 'this is a test' are not allowed prior to DEX version 040
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.
While that makes it a little bit harder to make a mistake here, it's still more like a reminder and it doesn't actually test if the written migration is valid or not
Ideally on top of that to have an actual "update from a previous version" test as one of the CI steps before release uploading
Thanks for the review, @kidinov! This PR is ready for another round 😄 |
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.
LGTM!
Description
This PR introduces unit tests that verify the integrity of database schema migrations. Previously generated schemas are hashed and compared to ensure no unintended modifications occur during the migration process. This helps prevent potential crashes due to schema inconsistencies.
Benefits:
Testing