-
Notifications
You must be signed in to change notification settings - Fork 290
ci: rework tests to break out ios/android tests #2519
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
Conversation
What do you think about also introducing flaky on the ios tests? |
We could, or there's an action that reruns on failure; actually I see at least two:
I've used one of those before, not sure which one. |
0f0c1de
to
442ca72
Compare
2c8868f
to
58e8df5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR restructures the CI test workflow to separate iOS and Android tests into dedicated test runs, while maintaining existing coverage for other platforms. The changes enable platform-specific testing by introducing a platform matrix parameter and updating test configuration to handle different build frontends.
- Split test matrix to explicitly separate iOS/Android from native platform tests
- Updated test fixtures to intercept build calls for Android and iOS platforms
- Modified CI workflow to conditionally run certain steps based on platform type
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
unit_test/main_tests/conftest.py | Added Android and iOS platform imports and build interception for testing |
.github/workflows/test.yml | Restructured test matrix to separate platform-specific runs and added conditional logic for different platforms |
ad42eb3
to
d6cf2cf
Compare
Weird, android is getting stuck in the unit_tests and hanging, while that should be identical to native. And macOS android is failing to download. Not sure what is causing this yet, though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The android unit test is probably failing due to something related to Docker - the docker tests are selected on that runner but binfmt isn't installed. I think my suggestion would be to not run the docker unit tests when testing android.
I haven't been able to figure out why the macOS android build is failing on Azure though.
@mhsmith there's a test failure here that you might be interested in? On Azure pipelines, macOS x86_64, the android runner seems to fail while installing the android ndk. The error looks like this:
It seems to just stop unzipping the archive and error out. The output's a bit cluttered because pytest is capturing it, I'll try to make it easier to read... |
Could the runner be running out of disk space while unzipping? I hit that problem on GHA, though I think the symptoms were different. I worked around it here: cibuildwheel/.github/workflows/test.yml Lines 74 to 75 in 3ebb6bd
|
Ah, it looks like I might have accidentally worked around it! I moved the first android test into the serial phase of the tests and it disappeared. So I'm thinking that maybe the issue was that the |
Ah, this might be because the NDK version specified in the android-env file is no longer pre-installed on the runners, so the android-env file now needs to install it. The NDK version will be updated in the next release of Python 3.13, but the versions on the runners change frequently, so the tests should be robust enough to cope with it not being present. It should be a sufficient workaround to run at least one Android test in the serial stage that gets as far as checking the NDK installation. This wasn't previously happening on runners that don't support the emulator, because the serial tests all require the emulator and were therefore skipped. Now that you've moved another test into the serial phase, it should be possible to remove the |
366aad9
to
d9fa4d5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR restructures the CI pipeline to separate Android and iOS tests from native platform tests, enabling parallel execution while maintaining fail-fast disabled behavior. The changes split the test matrix to run Android/iOS tests on specific platforms while keeping native tests running on all supported platforms.
Key changes:
- Test matrix separation for Android/iOS vs native platform tests
- Modified test runner to accept
--test-select
parameter for targeted test execution - Reorganized Android test ordering to ensure flaky initial setup test runs first
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
azure-pipelines.yml | Restructured job matrix to separate Android/iOS tests from native tests with conditional configurations |
.github/workflows/test.yml | Updated test matrix to include separate Android/iOS test runs with modified build frontend and platform settings |
test/test_android.py | Moved and marked the expected wheels test as serial and flaky to ensure proper test ordering |
Signed-off-by: Henry Schreiner <[email protected]>
Co-authored-by: Copilot <[email protected]>
Signed-off-by: Henry Schreiner <[email protected]>
Signed-off-by: Henry Schreiner <[email protected]>
Signed-off-by: Henry Schreiner <[email protected]>
Signed-off-by: Henry Schreiner <[email protected]>
Signed-off-by: Henry Schreiner <[email protected]>
Signed-off-by: Henry Schreiner <[email protected]>
Signed-off-by: Henry Schreiner <[email protected]>
This reverts commit aedf0f5.
Maybe it's a personal thing, but the header location was really making me have to think more than I needed to look at that file!
5209d1c
to
8b63508
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I think this is a great improvement to our CI setup.
Fail fast disabled, and trying to split out the android/ios runs.
The total time shouldn't be that much more than before, need to verify (a minute or two on sample builds is fine).