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

Double duration of awaitFinalized in ShadowThemeTest #8481

Merged
merged 1 commit into from Sep 23, 2023

Conversation

utzcoz
Copy link
Member

@utzcoz utzcoz commented Sep 23, 2023

Looks like shouldFreeNativeObjectInRegistry becomes flaky with JDK17 sometimes on GitHub Actions.

Looks like shouldFreeNativeObjectInRegistry becomes flaky with JDK17
sometimes on GitHub Actions.

Signed-off-by: utzcoz <utzcoz@outlook.com>
@utzcoz utzcoz requested a review from hoisie September 23, 2023 06:43
@utzcoz
Copy link
Member Author

utzcoz commented Sep 23, 2023

cc @hoisie .

@utzcoz utzcoz merged commit c6a97d6 into robolectric:master Sep 23, 2023
18 of 19 checks passed
@utzcoz utzcoz deleted the double-duration-of-awaitFinalized branch September 23, 2023 06:46
@utzcoz
Copy link
Member Author

utzcoz commented Sep 23, 2023

From testing result, looks like this PR reduces the flaky percentage, but this test case also flaky. @hoisie Maybe you can help to select a higher duration?

@hoisie
Copy link
Contributor

hoisie commented Sep 23, 2023

Guava has GCfinalization.awaitClear, maybe that works better?

We can also move this to MemoryLeaksTest which generates an hprof when run inside Google.

@utzcoz
Copy link
Member Author

utzcoz commented Sep 23, 2023

@hoisie I can give a try tomorrow. I just updated it again to 15s to make everything stable, at least it is stable with my some PRs testing.

@utzcoz
Copy link
Member Author

utzcoz commented Sep 24, 2023

@hoisie GCfinalization.awaitClear uses max 10s to wait the object to be GCed: https://github.com/google/guava/blob/4478b7f5ee8fbc63a3071b058abc876d2d192e85/guava-testlib/src/com/google/common/testing/GcFinalization.java#L117. But from my experience, 10s is not enough for the Robolectric's occasion.

@hoisie
Copy link
Contributor

hoisie commented Oct 7, 2023

This test was still flaky so I moved it to MemoryLeaksTest: c90c030

Internally at Google I ran it thousands of times and there wasn't a single flake.

I'll keep an eye on it.

@utzcoz
Copy link
Member Author

utzcoz commented Oct 7, 2023

@hoisie I think it is only flaky when running it on GitHub Actions environment. Maybe it is related to GitHub Actions environment's restriction as objects are recycled very late than expected not leaked. At least, we can retrigger it if flaky on GitHub Actions.

@hoisie
Copy link
Contributor

hoisie commented Oct 9, 2023

@utzcoz MemoryLeaksTest seems to run reliably on GitHub CI. That test uses Guava's GCFinalization assertions. I assume that Guava's GCFinalization probably handles some subtle edge cases that were not handled by the awaitFinalized in ShadowThemeTest.

@hoisie
Copy link
Contributor

hoisie commented Oct 9, 2023

In retrospect this may actually be a race condition. I know calling System.gc() will create a separate finalizer thread. Maybe there is a race between when this thread completes and when the assertions occur on the test thread.

@utzcoz
Copy link
Member Author

utzcoz commented Oct 10, 2023

@hoisie Maybe. The test suite of the memoryleak module is not large, and maybe it will cause flaky issues as you said. Could you merge google branch into master branch recently? I want the master branch to benefit from this change too.

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

2 participants