Skip to content

Default iOS tests to passed versions#549

Merged
swiple-rules-gardener merged 3 commits intobazelbuild:masterfrom
keith:ks/use-passed-versions
Oct 30, 2019
Merged

Default iOS tests to passed versions#549
swiple-rules-gardener merged 3 commits intobazelbuild:masterfrom
keith:ks/use-passed-versions

Conversation

@keith
Copy link
Copy Markdown
Member

@keith keith commented Aug 16, 2019

This makes the --ios_simulator_version and --ios_simulator_device flags,
affect which devices are used for testing if you don't otherwise provide
a device or OS version

This reverts commit 1a5bd8e.

@keith
Copy link
Copy Markdown
Member Author

keith commented Aug 16, 2019

blocked on bazelbuild/bazel#9191

@keith
Copy link
Copy Markdown
Member Author

keith commented Aug 27, 2019

For others who are interested in this you can use this now as long as you always pass some versions on these flags so it doesn't fall back to the very old defaults, we've been using this for months

@keith keith force-pushed the ks/use-passed-versions branch from 62dc22f to 571a382 Compare October 29, 2019 17:25
@keith
Copy link
Copy Markdown
Member Author

keith commented Oct 29, 2019

@thomasvl I think this should be good to merge now

@thomasvl
Copy link
Copy Markdown
Member

Hm, telling buildkite to rerun the tests to try and see what the failure was.

@keith
Copy link
Copy Markdown
Member Author

keith commented Oct 29, 2019

The failures definitely appear to be related to this change, but I'm not sure why ATM

@thomasvl
Copy link
Copy Markdown
Member

thomasvl commented Oct 29, 2019

Is your branch up to date? Maybe something else has changed, and I think BuildKite might try doing PRs off the branch point?

@keith
Copy link
Copy Markdown
Member Author

keith commented Oct 29, 2019

It is up to date yes

@keith
Copy link
Copy Markdown
Member Author

keith commented Oct 29, 2019

I'm not sure why this change would affect the default simulator version for UI tests since I removed the defaults in bazel itself and the tests aren't specifying a version, but if it did affect that the issue might be that 12.2 (which is what the tests are trying to use) isn't from the xcode-select'd Xcode on the CI machines, and it isn't installed globally, but it is the default version for the --xcode_version being used (although this shouldn't be the case since we're not specifying one.

@thomasvl
Copy link
Copy Markdown
Member

Seeing the failures on other things now also, so it isn't this cl. I'll return to this after figuring out what's up on BuildKite.

keith added 2 commits October 30, 2019 08:39
This makes the --ios_simulator_version and --ios_simulator_device flags,
affect which devices are used for testing if you don't otherwise provide
a device or OS version

This reverts commit 1a5bd8e.
@keith keith force-pushed the ks/use-passed-versions branch from edb69d4 to 20e5267 Compare October 30, 2019 15:39
@keith
Copy link
Copy Markdown
Member Author

keith commented Oct 30, 2019

@thomasvl all green with that CI fix!

@thomasvl
Copy link
Copy Markdown
Member

Running this thru internal testing also.

But now that I think about it, shouldn't this also be done to the tvOS ones? Since you made to make a change to bazel for this, would the tvOS ones also need the same work?

@keith
Copy link
Copy Markdown
Member Author

keith commented Oct 30, 2019

So actually right now those aren't exposed to starlark. From reading the code it appears to be because bazel wants to deprecate the individual accesses for https://github.com/bazelbuild/bazel/blob/a330febdddbae5751b6bb0cc0a6a84f451df2304/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/apple/ObjcConfigurationApi.java#L62-L74 instead, which rules_apple doesn't currently consume at all.

But yes, in theory we should, but also right now tvOS tests (publicly) are a no-op 😄 https://github.com/bazelbuild/rules_apple/blob/97951dce38d36316e870950eaa0c7a5583353a87/apple/testing/default_runner/tvos_test_runner.template.sh

@thomasvl
Copy link
Copy Markdown
Member

Oh, didn't even know about those, add another thing on the queue to look at updating too…

@keith
Copy link
Copy Markdown
Member Author

keith commented Oct 30, 2019

I think we could easily change the API for the iOS ones here too and then remove the iOS exposed versions from bazel which would probably be nice

swiple-rules-gardener added a commit that referenced this pull request Oct 30, 2019
@swiple-rules-gardener swiple-rules-gardener merged commit d05d990 into bazelbuild:master Oct 30, 2019
@keith keith deleted the ks/use-passed-versions branch October 30, 2019 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants