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

Add local cache unit test for long async refresh case #7038

Closed
wants to merge 1 commit into from

Conversation

paggynie
Copy link
Contributor

This is a follow up on #6851 about adding unit test to avoid similar breaking change happen again. This is a generic test to test async refresh on another thread takes long enough for entry to be expired and we still expect to view the reload result without any exception.

I verified with change introduced from #6851 , the test failed with

Tests run: 9571, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 1.677 sec <<< FAILURE!
testLongAsyncRefresh(com.google.common.cache.LocalCacheTest)  Time elapsed: 0.102 sec  <<< ERROR!
java.lang.IllegalStateException: Recursive load of: test

after the change from #6851 roll back, the test passes

Running com.google.common.cache.LocalCacheTest
Tests run: 9571, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 4.962 sec

@cpovirk cpovirk self-assigned this Feb 26, 2024
@cpovirk cpovirk added type=other Miscellaneous activities not covered by other type= labels package=cache P2 labels Feb 26, 2024
copybara-service bot pushed a commit that referenced this pull request Mar 14, 2024
…rite duration.

(followup to cl/605069776 / #6851)

Also, restore the use of `ConcurrentMapTestSuiteBuilder` in the mainline. It was added in cl/94773095 but then lost during cl/132882204 in the mainline only.

Fixes #7038

RELNOTES=n/a
PiperOrigin-RevId: 610388763
@cpovirk
Copy link
Member

cpovirk commented Mar 15, 2024

@paggynie, my internal reviewer ended up challenging me to rewrite this test in a way that avoided sleep and otherwise guaranteed that it would catch the bug, no matter what happened with scheduling. I think I managed that in d5fbcca, but if anything worries you about my changes, let me know. I still left the PR credited to you, since you reported the bug and provided the PR that I started from. But if anyone tells you there was something wrong with the commit that we merged with your name on it, you can point to my explanation here :) Sorry for taking so long on this, and sorry again for the bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 package=cache type=other Miscellaneous activities not covered by other type= labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants