build(tools): introduce cocoapods patch #9322
Conversation
There was a problem hiding this comment.
if some of our forks are small changes they might be better as patches
curious on the why though 馃憖, will it potentially speed up ci steps?
we need to give some attention to these ancient pods 馃憖
Def, and I bet that we have a bunch of unused code there as well 馃榿
I am basically thinking we are more likely to update a patched pod then a forked pod, so a lot of times creating a fork essentially freezes our dep in time. updating forks requires us to sync with upstream and is kind of ambiguous and scary. Updating a patched one, you have to reapply the patch for any changes but easier IMO. Ideal though is to not do either 馃槄 and get any changes we need merged back upstream, not always practical though. |
|
@brainbicycle - semi-controversial, but these tests have served their purpose and will never be updated. Old tests aren't valuable. Just delete them, don't patch our package manager (!) |
These snapshots are used in almost all our native tests unfortunately, I think deleting that many is a big step, the snapshot tests it is true aren't updated often but do catch breakages in native code from time to time, live auctions especially. I do agree with you I don't want patches around, I see this as a stepping stone until we put some dedicated time into getting our native pods up to date or getting rid of them. |
|
Q: what does the unit test coverage look like in said apps? Folks have always been pretty diligent about test coverage, and i'm certain it wasn't all snaps. How necessary is this particular layer? |
We relied on them pretty heavily for UI related testing: https://github.com/artsy/eigen/blob/f0b572bce8a23dddd97a42a30d89707a70cd4688/ios/ArtsyTests/View_Tests/Buttons/ARBidButtonTests.m |
|
Looking here: https://github.com/search?q=repo%3Aartsy%2Feigen%20haveValidSnapshotNamed&type=code The coverage is very (very) minimal app-wide! Lets just write a couple unit tests for those areas of the code (in the best case), and in the worst case just delete these snaps. It so small we'll be fine and if we're not, there's exactly three components we'd need to look at, because only three tests use this library. (I also just checked the other api methods from the lib and we're only using that one -- no other uses.) |
we use haveValidSnapshot as well, Xcode reports 54 results across 18 files, not as bad as I thought but still not a trivial update. Also worth mentioning the snapshots are not the only thing being patched, we had what was essentially inline patches in the podfile earlier for compilation errors. In my mind to get to a place to not have these we need to audit the native code + deps and remove unnecessary code and then we can safely remove most of the tests. |
|
Chatted in slack, all good 馃憤 |
|
Hey @brainbicycle 馃憢 Hope you're well. Apologies for not releasing an update to Nimble-Snapshots sooner. cocoapods-patch is a cool idea. FYI I've released the update for Xcode 15 in version 9.6.0. Take care! |
|
hey @ashfurrow ! all good here, thanks a ton! it is on us for not keeping our Pods up to date, giving them some attention soon though 馃檹 Thanks for the fix in any case, hope all is good with you! |
This PR resolves []
Description
Stepping stone on way to Xcode 15, was working on the upgrade and got some compilation failures in tests due to this:
ashfurrow/Nimble-Snapshots#268
Unfortunately that has not yet been released + we are way behind on versions so it is going to take some effort to get there even when it is.
I think our goal should be to have 0 of these but I think for the time being we need them and at least we can keep them organized in one place.
This dep works similarly to patch-package:
https://github.com/doublesymmetry/cocoapods-patch
Potential follow-ups
PR Checklist
To the reviewers 馃憖
Changelog updates
Changelog updates
Cross-platform user-facing changes
iOS user-facing changes
Android user-facing changes
Dev changes
Need help with something? Have a look at our docs, or get in touch with us.