wordpress-mobile / AztecEditor-iOS

A reusable native iOS visual HTML text editor component.
Mozilla Public License 2.0
612 stars 146 forks source link

[Tooling] Make `pod trunk push` be `--synchronous`, to fix issue when releasing both pods together #1391

Open AliSoftware opened 5 months ago

AliSoftware commented 5 months ago

Why?

To prevent the Editor pod to fail to be pushed to trunk if the Aztec pod it depends on hasn't propagated through the CDN yet

Fixes the issue raised in this Slack thread: p1711026617605489-slack-CC7L49W13 cc @geriux

Testing

It's gonna be difficult to test the change on this repo without actually doing a new version of the Aztec and Editor pods.

The new ability of publish_pod to accept a --synchronous flag has already been tested as part of the Gravatar-iOS-SDK repo (which had a similar issue of co-dependant pods having to be released together), so I think we should be good though.

So I say we can probably trust that it will work as expected like it did for the Gravatar repo, and I'm counting on the next dev who will have to make a new version of these pods to let us know if it ends up still not working.

mokagio commented 5 months ago

I noticed CI has been waiting for almost 12 hours.

Not surprising given it's configured to run on Xcode 13 😳

image

Looking at upgrading now...

AliSoftware commented 5 months ago

It seems like the test failures are due to the "Allow Paste" System Prompt—which now appears on the simulator in more recent iOS versions—during the copy/paste-related tests.

@geriux @mokagio can I let you look at those failures? There's probably a way in XCTest to catch those during a UITest (maybe addUIInterruptionMonitor? Other?) I gave it a quick try on my end but this quickly turned into a rabbit hole and I didn't have the bandwidth to go further on my end, so I'll let this to you 🙇


PS: While taking a quick look at the workspace I noticed that the build settings are still set to DEPLOYMENT_TARGET=11.0 (and the podspec as well), and if we update the project to e.g. iOS13, there are a couple of depreciations warnings and changes that would be needed in the code to get rid of those warnings (which would otherwise prevent the pod to be released) + make the lib ready for later updates.

I also noticed that all the methods around manipulating and converting between NSRange/UTF16NSRange/Range<String.UTF16View> seems from a time where the model for the String type in Swift was not as nice as today's. Since then, Swift.Foundation have gotten a lot of built-in methods for that (like NSRange(swiftrange, in: swiftstring) and the likes) which would be more resilient (including handling edge cases like NSNotFound and how it might be represented differently in 32 vs 64 bits platforms, ranges with negative locations and/or length, etc) than the manual code written in Aztec's extensions, and switching to them would make the code more bug-proof, especially on such a tricky topic of manipulating Unicode representations of Strings.

mokagio commented 5 months ago

PS: While taking a quick look at the workspace I noticed that the build settings are still set to DEPLOYMENT_TARGET=11.0 (and the podspec as well), and if we update the project to e.g. iOS13, there are a couple of depreciations warnings and changes that would be needed in the code to get rid of those warnings (which would otherwise prevent the pod to be released) + make the lib ready for later updates.

2f50d83de2c2d6d2c9f6ed7f9ec73eb0fa9d3e4e bumps the target to iOS 12, which is the minimum supported by Xcode 15. The warnings remain, I added --allow-warnings to the pod commands as a workaround.

mokagio commented 5 months ago

It seems like the test failures are due to the "Allow Paste" System Prompt—which now appears on the simulator in more recent iOS versions—during the copy/paste-related tests.

🤔

@jkmassel addressed this in #1377 by making the pasteboard injectable during unit testing. When I merged the latest develop into #1392, the lockup related to prompt disappeared.

As far as I can tell, the remaining tests failures at the custom TextView level are due to something misbehaving during the HTML parsing. If I had time to investigate, I would pursue the String API route you suggested:

I also noticed that all the methods around manipulating and converting between NSRange/UTF16NSRange/Range seems from a time where the model for the String type in Swift was not as nice as today's

AliSoftware commented 5 months ago

The warnings remain, I added --allow-warnings to the pod commands as a workaround.

I don't see that --allow-warnings added to the pod commands in .buildkite/ yet, so I'm guessing you either wrote that you'd do it but forgot, or forgot to push the change 😄