Skip to content
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

Use Xcode 15.3 in CI #1392

Open
wants to merge 20 commits into
base: tooling/fix-sync-pod-publication
Choose a base branch
from

Conversation

mokagio
Copy link
Contributor

@mokagio mokagio commented Apr 16, 2024

Builds on top of #1391, where I noticed that CI didn't run because we no longer have agents with Xcode 13 online.

To test: See green CI running on Xcode 15.3 image... via #1393 which marks four tests as expected failures 😞


  • I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary.

@mokagio mokagio mentioned this pull request Apr 16, 2024
2 tasks
@mokagio mokagio marked this pull request as draft April 16, 2024 06:29
@mokagio
Copy link
Contributor Author

mokagio commented Apr 16, 2024

The test that locks is testCopyAndPasteToPlainText from TextViewTests. It didn't occur for me locally till I tried to run the test class individually.

@mokagio
Copy link
Contributor Author

mokagio commented Apr 16, 2024

The test that locks is testCopyAndPasteToPlainText from TextViewTests. It didn't occur for me locally till I tried to run the test class individually.

Figured out why...

Screenshot 2024-04-16 at 19 49 34

@mokagio
Copy link
Contributor Author

mokagio commented Apr 16, 2024

Ah! I also realized that @jkmassel had already worked out that issue in #1377. I had seen some code about UIPasteboard .forTesting and was surprised not to find it on my local. I then realized this branch was out of sync with develop.

mokagio and others added 2 commits April 17, 2024 13:40
@mokagio This should fix the `testPaste{Image,Video}WithoutFormatting` test failures.
 - I have searched the code for `decodeObject(forKey` occurrences and confirmed there wasn't any left
 - But I have not triple-checked that *every* custom subclass of anything we might encode in our `NSAttributedString` attributes have had `+supportsSecureCoding` redefined

Indeed, apparently, **even if** the parent class already overrides it to return `true`, subclasses which override `init?(coder:)`/`encode(with:)` from their parent class need to also re-override `+supportsSecureCoding`. I've added the overrides in classes that I've modified, but there may be more classes that might not be covered by our unit tests around archiving/pasting but would still require it to be added? So would be worth making another pass to be sure we didn't forget any,
@AliSoftware
Copy link
Contributor

AliSoftware commented Apr 18, 2024

@mokagio I took the liberty to push a commit (665b15b) to fix the testPaste{Image,Video}WithoutFormatting test failures.

[EDIT]I later fixed that commit 665b15b with c67821c[/EDIT]


  • I have searched the code for decodeObject(forKey: occurrences and confirmed that I've replaced all of them with decodeObject(of: …, forKey: …) instead
  • But I have not triple-checked that every custom subclass of anything we might encode in our NSAttributedString attributes have had +supportsSecureCoding redefined

Indeed, apparently, even if the parent class already overrides it to return true, subclasses which override init?(coder:)/encode(with:) from their parent class need to also re-override +supportsSecureCoding.

I've added the overrides in classes that I've modified, but haven't check if maybe there are more classes that might not be covered by our unit tests around NSAttributedString archiving/pasting, but would still require it to be added. So would be worth making another pass to be sure we didn't forget any (otherwise the client app would start dismissing those attributes during pasting, while it didn't before our migration to iOS12).

…inText`

Since newer versions of iOS use this new `public.utf8-plain-text` UTI instead of `public.plain-text` like in previous OS versions.
This reverts commit 5c3004b, because I'm not convinced that's a good idea.

We should double-check that **not** having `+supportsSecureCoding` declared on this subclass doesn't make it fail to be copy/pasted (i.e. archived/unarchived)—like similar cases happened when running Aztec tests. Or if we need `+supportsSecureCoding`, we need to find a way to override/redefine it across module boundaries…
@AliSoftware
Copy link
Contributor

AliSoftware commented Apr 18, 2024

@mokagio Just also pushed 90c0e4d to fix the testCopyAndPasteToPlainText test failure on extra \n.

⚠️ That being said, while the test pass, I'm still not 100% sure why there was a \n when we were using public.plain-text before, so maybe there's more to it and it could still be worth investigating further (and add more tests to cover the cases we missed if needed)

There's also still another test failing, this time in WordPressEditor where testCaptionShortcodeIsProperlyConvertedIntoFigureTagPreservingNestedTags() seems to have an extra space in the output. I haven't looked into that one so I'll leave it to you to take a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants