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

Bash cache & UIPasteBoard tests updates #686

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from
Open

Conversation

mokagio
Copy link
Contributor

@mokagio mokagio commented Oct 4, 2022

This PR started as one to upgrade the project to build using the xcode-14 image in CI. However I didn't iterate on it fast enough and other PRs (#695, #699) did most of the work. So, this PR now only tracks minor improvements.

@mokagio mokagio marked this pull request as ready for review October 4, 2022 05:33
@mokagio mokagio changed the base branch from trunk to mokagio/bash-cache-update October 4, 2022 05:33
@mokagio mokagio marked this pull request as draft October 4, 2022 05:33
Base automatically changed from mokagio/bash-cache-update to trunk October 4, 2022 08:17
@mokagio mokagio force-pushed the mokagio/xcode-14 branch 2 times, most recently from 8e024f5 to dce86f2 Compare November 7, 2022 05:17
This new version has an improved Ruby Bundler management.
@mokagio mokagio changed the title Update codebase and CI setup for Xcode 14 Bash cache & UIPasteBoard tests updates Nov 7, 2022
@mokagio mokagio marked this pull request as ready for review November 7, 2022 11:43
@mokagio mokagio requested review from selanthiraiyan and a team November 7, 2022 11:43
Comment on lines +17 to +24
// FIXME: We'll need to find a way to make the test work the new pasteboard rules
//
// See:
// - https://developer.apple.com/forums/thread/713770
// - https://sarunw.com/posts/uipasteboard-privacy-change-ios16/
// - https://github.com/wordpress-mobile/WordPressAuthenticator-iOS/issues/696
XCTExpectFailure("Paste board access has changed in iOS 16 and this test is now failing")

Copy link
Contributor

@AliSoftware AliSoftware Nov 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we happen to run the Unit Tests using an iPhone Simulator that runs iOS 15.4 or older? Shouldn't we guard this against the iOS runtime version?

Probably with something like this:

Suggested change
// FIXME: We'll need to find a way to make the test work the new pasteboard rules
//
// See:
// - https://developer.apple.com/forums/thread/713770
// - https://sarunw.com/posts/uipasteboard-privacy-change-ios16/
// - https://github.com/wordpress-mobile/WordPressAuthenticator-iOS/issues/696
XCTExpectFailure("Paste board access has changed in iOS 16 and this test is now failing")
// FIXME: We'll need to find a way to make the test work the new pasteboard rules
//
// See:
// - https://developer.apple.com/forums/thread/713770
// - https://sarunw.com/posts/uipasteboard-privacy-change-ios16/
// - https://github.com/wordpress-mobile/WordPressAuthenticator-iOS/issues/696
if ProcessInfo.processInfo.isOperatingSystemAtLeast(.init(majorVersion: 16, minorVersion: 0, patchVersion: 0)) {
XCTExpectFailure("Pasteboard access has changed in iOS 16 and this test is now failing")
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed this is needed, by testing on an iOS 15.4 simulator 😉
image

Copy link
Contributor Author

@mokagio mokagio Nov 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat technique.

But do we really need it? Yes, the tests will fail on iOS < 16, but how much of a concern is that for us? What are the chances we'll need to run the tests in iOS < 16?

Thinking about it, it might be beneficial if the UIPasteboard changes are such that we'll need different code between iOS < 16 and >= 16. 🤔 I haven't looked at that yet, this might be a good time to do so.

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