Replace "full" iterative CoreData migration test with "130 to latest" test instead, to speed up test suite #18668
Conversation
That test took too long to run... A 130% test run time increase in CI! This new version takes 3s on an Intel 2019 Mac, which is still a long time, but not at the scale of the previous one. See also comments in the code for more details.
| // The `_` at the start of the method makes it so that the XCTest runner will not pick it up as | ||
| // a test to run. | ||
| // | ||
| // It's not practical to run this test every time because it walks through 100+ migrations and | ||
| // takes 90 seconds (Intel MacBook Pro 2019)! | ||
| // | ||
| // We're keeping the code here just in case we'll ever need to test the full migration flow. | ||
| func _testIterativeMigration19ToLatest() throws { |
There was a problem hiding this comment.
I'd be fine deleting this, too. I can't think of a scenario in which we might want to run this kind of test.
You can test the changes in Jetpack from this Pull Request by:
|
You can test the changes in WordPress from this Pull Request by:
|
| // At the time of writing we are at app version 19.9 and model version 140. | ||
| // At app version 19.0 we were at model version 137. | ||
| // Iterating back 10 version is more than plenty to cover a real world scenario. | ||
| try prepareForMigration(withModelName: "WordPress 130") { context in |
There was a problem hiding this comment.
Eventually, I'd like to do something dynamic where we pick up current - x versions. But in that case we wouldn't be able to write a test like this because obviously if it's dynamic we don't know how the base version differs from the latest.
I do wonder whether we need this level of detail in the test in the first place 🤔
This new test follows the template of the previous one, but maybe it's enough to run the migrations and make sure they don't crash, then leave testing the details to the ad hoc test for "migration from x to y?
Anyways, that's something we can tackle in a followup. For the moment I'm just keen on bringing the test suite back to its previous run time.
There was a problem hiding this comment.
That means that in a couple of months and versions we'll be back to this test making more and more migrations, the more we add new versions to our xcdatamodel though…
Though limiting this test to, say, 130–137 instead of 130–current would be useless (i.e. once we proved that it passed for 130–137 there's no reason this test would ever fail after we add models 138 and later, since by design of xcdatamodeld versioning, we never change older models…), and limiting to (current - 10)–current as you said would not work because it would be too dynamic as well… So not sure there's an ideal solution there …
I guess we could live with 130–current for now, and consider revisiting in a year or so when we'd have a lot of new xcdatamodel versions piled on top of 137, again increasing the test duration too much.
Maybe we could use the "Expiring Todo" SwiftLint rule to track that and be reminded about it 1 year from now?
There was a problem hiding this comment.
This change looks good to me. 👍
@mokagio Just an idea for the dynamic migration approach you mentioned. Instead of figuring out the differences between the old and latest version, and making assertions based on that, we could assert the database file has indeed been migrated to the latest version. For example, at the end of this test, after migration is completed, we could get NSManagedObjectModel of the database file, which is implemented here, and assert it is the latest version.
|
@crazytonyli I can't add you to the reviewers list because of the org membership, but I'd love to know what you think of this—if you have time for it. Thanks! |
AliSoftware
changed the title
There was a problem hiding this comment.
Approving to unblock and because the solution presented is still way better than having an extra-long Unit Test that was not really worth testing the full path anyway.
We might still need to find a solution (expiring todo via SwiftLint?) so that the long-time issue does not come back after a couple of months and new xcdatamodel versions, and we might want to explore the alternative idea that @crazytonyli suggested in his comment here at some point.
Feel free to keep this PR open and use it to continue exploring the various ideas (like those 2 above) further, but I thought I'd still approve it in its current state to allow you to merge it as is + open subsequent PR(s) later to experiment on the alternative ideas when you have the time, if you preferred to go that route.
Annnnd shoot I missed that auto-merge was enabled on the PR when I commented this and approved it 😅 |
Will do! #18797 |
That test, which had been in the suite for a while but never run properly till #18634, took too long to run... A 130% test run time increase in CI!
This new version takes 3s on an Intel 2019 Mac, which is still a long time, but not at the scale of the previous one.
See also comments in the code for more details.
Hat tip @crazytonyli for the detective work and the conversation prompted this PR 🎩 ✨
To Test
Green CI should be enough, given this is purely a testing and test run time change.
You can also verify the build on this branch too ~7m for the tests, while on
trunkthey currently run at ~9mFrom Fastlane's test report attached in CI for this branch:
While on
trunk:Regression Notes
RELEASE-NOTES.txtif necessary. N.A.