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

Import Robolectric Native Graphics tests #7997

Merged
merged 0 commits into from
Mar 20, 2023
Merged

Conversation

hoisie
Copy link
Contributor

@hoisie hoisie commented Mar 2, 2023

Import Robolectric Native Graphics tests

Create a new integration_test project, 'nativegraphics', to contain these tests.
The advantage of an integration test project is that it can use AGP and have
access to resources.

@utzcoz
Copy link
Member

utzcoz commented Mar 16, 2023

@hoisie Looks like https://android-review.googlesource.com/c/platform/external/robolectric/+/2492289 AOSP wants to merge this PR, what about merging it first?

@ihcinihsdk
Copy link

@hoisie @utzcoz Hi Michael and utzcoz,
I was creating https://android-review.googlesource.com/c/platform/external/robolectric/+/2492289 to work on enable the test, and was planning to wait for the merge of this PR and grab the code via git fetch / pull etc.

Having said that, do you have an ETA for merging this PR?

@hoisie
Copy link
Contributor Author

hoisie commented Mar 17, 2023

Hi @ihcinihsdk working on this now :)

@ihcinihsdk
Copy link

Hi @ihcinihsdk working on this now :)

Thanks a lot, Michael!

@hoisie hoisie changed the title Import Robolectric native graphics tests Import Robolectric Native Graphics tests Mar 17, 2023
@hoisie hoisie force-pushed the piper_513278903 branch 2 times, most recently from e1178ed to a1c60e6 Compare March 17, 2023 07:42
@utzcoz
Copy link
Member

utzcoz commented Mar 17, 2023

@hoisie Does RNG support webp? I remember there is a webp supporting problem of previous resource mechanism.

@hoisie
Copy link
Contributor Author

hoisie commented Mar 17, 2023

@utzcoz yup it supports WebP

@hoisie hoisie force-pushed the piper_513278903 branch 2 times, most recently from 95eb39d to 11a7f07 Compare March 17, 2023 19:49
@@ -64,8 +64,20 @@ jobs:
--no-watch-fs \
-Drobolectric.enabledSdks=${{ matrix.api-versions }} \
-Drobolectric.alwaysIncludeVariantMarkersInTestName=true \
-Dorg.gradle.workers.max=2 \
-x :integration_tests:nativegraphics:test
Copy link
Member

Choose a reason for hiding this comment

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

@hoisie Is it for test purpose only?

Copy link
Contributor Author

@hoisie hoisie Mar 18, 2023

Choose a reason for hiding this comment

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

It's complicated... I was seeing weird CI failures when running native graphics tests in the regular tests CI workflow. Seems too be a kind of OOM. the gradle test will fail with 'exit code 137'. If I separate the graphics tests so they are in a separate process it fixes the issue.

I am thinking of running the graphics tests on our M1 mini runner for now.

We may also upgrade to a paid plan to use larger CI machines with more than 7 GiB ram.

Copy link
Member

Choose a reason for hiding this comment

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

Oh. Looks like all tests consume too much RAM. It's great to separate native graphics tests.

We may also upgrade to a paid plan to use larger CI machines with more than 7 GiB ram.

Is it necessary? At least current CI time is accepted after nativeruntime building removed. We can trade time for space now.

@@ -64,8 +64,20 @@ jobs:
--no-watch-fs \
-Drobolectric.enabledSdks=${{ matrix.api-versions }} \
-Drobolectric.alwaysIncludeVariantMarkersInTestName=true \
-Dorg.gradle.workers.max=2 \
-x :integration_tests:nativegraphics:test
Copy link
Member

Choose a reason for hiding this comment

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

Oh. Looks like all tests consume too much RAM. It's great to separate native graphics tests.

We may also upgrade to a paid plan to use larger CI machines with more than 7 GiB ram.

Is it necessary? At least current CI time is accepted after nativeruntime building removed. We can trade time for space now.

@utzcoz
Copy link
Member

utzcoz commented Mar 18, 2023

@utzcoz yup it supports WebP

Cool. The RNG is awesome. Looks like this PR can be merged because CI passed.

@hoisie
Copy link
Contributor Author

hoisie commented Mar 18, 2023

@utzcoz I'll merge it via copybara, needs to get a couple other approvals first.

Yeah it does seem like Error 137 comes is triggered by the GitHub actions host machine when it kills the process due to exceeding memory thresholds:

https://stackoverflow.com/questions/68844666/github-action-is-being-killed

We have been occasionally seeing this error 137 in other places, not just RNG tests.

Robolectric's test suite is very large and complex, it's pretty amazing it that it has been running in under 7 GiB of RAM for so long.

I think the plan is to experiment with larger machines for a while, and if the cost is reasonable, we can go with the ones with 16 GiB ram.

@copybara-service copybara-service bot force-pushed the piper_513278903 branch 2 times, most recently from b5a52cd to f77bf44 Compare March 20, 2023 18:23
@copybara-service copybara-service bot closed this Mar 20, 2023
@copybara-service copybara-service bot merged commit b40b431 into google Mar 20, 2023
@copybara-service copybara-service bot deleted the piper_513278903 branch March 20, 2023 18:48
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.

None yet

3 participants