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

[Fix] Include iOS tests #341

Merged
merged 21 commits into from
Oct 6, 2021
Merged

[Fix] Include iOS tests #341

merged 21 commits into from
Oct 6, 2021

Conversation

JJ
Copy link
Contributor

@JJ JJ commented Oct 4, 2021

Warned by @fhd, iOS tests were silently failing, because the platform requested was missing. This is one of the platforms listed, let's give it a try.

@JJ JJ requested a review from fhd October 4, 2021 09:07
@JJ JJ changed the title [Fix] Use one of the available destinations [Fix] Use one of the available iOS destinations Oct 4, 2021
@JJ
Copy link
Contributor Author

JJ commented Oct 4, 2021

Well, now we've improved one notch. It's still silently failing, but with what looks like a genuine test failure. I wonder if we should stop using make for this and change to cough fastlane cough?

@fhd
Copy link
Contributor

fhd commented Oct 4, 2021

It's not make - if you leave out the xcpretty call it fails as expected. The latter swallows the failure, however. It's an xcpretty problem, but I'm working on a little workaround.

@fhd
Copy link
Contributor

fhd commented Oct 4, 2021

@JJ This should do it: 693fdbb

So I expect I've just provoked a build failure in main, and your PR here should fix it.

ios/Makefile Outdated
@@ -12,7 +12,7 @@ test:
xcodebuild clean test \
-project $(project_path) \
-scheme $(scheme_name) \
-destination "platform=iOS Simulator,name=iPhone 11,OS=13.3"
-destination "platform=iOS Simulator,name=iPhone 11,OS=14.4"
Copy link
Contributor

Choose a reason for hiding this comment

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

That's not quite ideal, since iOS 13.3 is our lowest supported version. Would be good to see if we can't get the older emulator back on GitHub's workers... But it's still better than a failing build if we test only 14.4.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that there does not seem to be any other OS available. These are the platforms:

	{ platform:iOS Simulator, id:B7C37619-BA36-424D-B938-132F83363FCD, OS:14.4, name:iPad (7th generation) }
		{ platform:iOS Simulator, id:AFBB8BA9-2CE0-4D7F-999B-770713FD10EF, OS:14.4, name:iPad (8th generation) }
		{ platform:iOS Simulator, id:9EB83B11-C00F-4F8A-925C-BA3619BB931B, OS:14.4, name:iPad Air (3rd generation) }
		{ platform:iOS Simulator, id:9CD40043-30E1-4E28-B8F5-EF198D55D3A2, OS:14.4, name:iPad Air (4th generation) }
		{ platform:iOS Simulator, id:75815B2E-87B9-4BD8-AE35-915EA4112D36, OS:14.4, name:iPad Pro (9.7-inch) }
		{ platform:iOS Simulator, id:1812C170-B3E3-406B-BD19-D759B8916FD5, OS:14.4, name:iPad Pro (11-inch) (2nd generation) }
		{ platform:iOS Simulator, id:9D4B1AEE-8EE8-49DE-9BEE-1A5E7D4E3507, OS:14.4, name:iPad Pro (12.9-inch) (4th generation) }
		{ platform:iOS Simulator, id:2FD5C51E-131F-42EF-8F68-8DCF6389144E, OS:14.4, name:iPhone 8 }
		{ platform:iOS Simulator, id:D6B9EF96-4E31-415E-8990-F2AD3F0BDAB5, OS:14.4, name:iPhone 8 Plus }
		{ platform:iOS Simulator, id:EAD2AC5A-D0FE-41C0-8F70-FBBC7460CF69, OS:14.4, name:iPhone 11 }
		{ platform:iOS Simulator, id:003786C0-2A5E-412B-A9F1-10A664FD93EC, OS:14.4, name:iPhone 11 Pro }
		{ platform:iOS Simulator, id:C78245F3-BC29-4E8B-84D1-EA1D69893C7B, OS:14.4, name:iPhone 11 Pro Max }
		{ platform:iOS Simulator, id:B483A12A-3A4D-44A4-949E-5E5B523B8A1C, OS:14.4, name:iPhone 12 }
		{ platform:iOS Simulator, id:B4072E75-75F3-459C-92AB-A3ECD5F4CDAA, OS:14.4, name:iPhone 12 Pro }
		{ platform:iOS Simulator, id:3163E6F7-9CFA-45B7-AA7D-C51484A8D5BE, OS:14.4, name:iPhone 12 Pro Max }
		{ platform:iOS Simulator, id:156CDD5C-0B1F-4E5F-A8CA-42D5B98FB663, OS:14.4, name:iPhone 12 mini }
		{ platform:iOS Simulator, id:7AE89782-2796-4CF4-9020-E40E3B43E52F, OS:14.4, name:iPhone SE (2nd generation) }
		{ platform:iOS Simulator, id:10E0C3AF-82E2-4623-84C1-4F59B67CEC39, OS:14.4, name:iPod touch (7th generation) }

If you want, I can try and see what's the origin of those destinations before merging. It might simply be that it's implicit with the macos-latest runner. We might want to create a matrix to run other versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what came up with. Apparently just one version is installed by default, so we can either try that or else check what versions are installed in other macos runners.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'm missing something here. Theoretically "latest" runner should be 10.15, and that should support 13.3. The Catalina runner, which should not be an alias of latest, only supports 13.7 and 14.4. So I might want to try to set multiple OS here, with different OS versions. Shouldn't be hard to do with Makefiles. We can also leave that for later if you want.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally we'd run the tests on multiple versions, but if we can somehow influence what runner we get (so that it has 13.3), that'd be good enough for now I'd say! If we can have multiple runners, to be economical, I would suggest we go for the lowest supported version (13.3) and the latest (15.0?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll see what I can do. Apparently, 15.0 is supported

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And right, latest started using macOS-11 last month, and it hadn't yet bubbled up to the documentation. Let me run a couple of tests to see if what you say above can be done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hum, no, it's using 10.15 all the time.

@JJ
Copy link
Contributor Author

JJ commented Oct 5, 2021

OK, this is set up as indicated, except one of the jobs is being cancelled when the other fails. Theoretically, it should test at the same time both destinations, 13.3 and 14.4. Please take a look @fhd.

@JJ JJ changed the title [Fix] Use one of the available iOS destinations [Fix] Create a strategy matrix for iOS Oct 5, 2021
@JJ JJ changed the title [Fix] Create a strategy matrix for iOS [Fix] Include iOS tests Oct 6, 2021
@JJ
Copy link
Contributor Author

JJ commented Oct 6, 2021

This is 🟢 and ready for review and/or merge.

JJ pushed a commit that referenced this pull request Oct 6, 2021
In this case, I've set one that will never be triggered, actually, until #341 is
merged.

Pushing directly to main to avoid errors in today's branches
Copy link
Contributor

@fhd fhd left a comment

Choose a reason for hiding this comment

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

BEAUTIFUL!

@JJ JJ merged commit db3cf65 into main Oct 6, 2021
@JJ JJ deleted the fix/ios-platform-error branch October 6, 2021 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants