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

Fix flaky SlidingWindowTest #1242

Merged

Conversation

Stephan202
Copy link
Contributor

@Stephan202 Stephan202 commented Dec 22, 2024

Observed in the context of PicnicSupermarket/error-prone-support#1468, this PR aims to resolve the following transient test failure:

[ERROR] io.prometheus.metrics.core.metrics.SlidingWindowTest.rotate -- Time elapsed: 0.023 s <<< FAILURE!
org.opentest4j.AssertionFailedError:
[Start time: 1734782202868, current time: 1734782232878, elapsed time: 30010]
expected: [1.0, 1.0, 1.0, 1.0, 1.0]
 but was: [1.0, 1.0, 1.0, 1.0]
    at io.prometheus.metrics.core.metrics.SlidingWindowTest$Observer.assertValues(SlidingWindowTest.java:30)
    at io.prometheus.metrics.core.metrics.SlidingWindowTest.rotate(SlidingWindowTest.java:57)
    at java.base/java.lang.reflect.Method.invoke(Method.java:569)
    at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
    at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

This fix likely also resolves #956.

How to reproduce:

mvn install -DskipTests -pl prometheus-metrics-core -am
while mvn test -Dtest='SlidingWindowTest' -pl prometheus-metrics-core; do echo Again...; done

Prior to this fix the loop exists after a few minutes (or even significantly faster), while with this change the loop does not appear to terminate.

@Stephan202
Copy link
Contributor Author

Stephan202 commented Dec 22, 2024

Alight, so somehow I failed to rerun the full test class; I could swear I did that before opening the PR. 🤦 I'm having a closer look at this issue and will try to propose a fix that doesn't cause the other test to fail. Sorry for the noise! Will close PR until I have a proposal.

@Stephan202 Stephan202 closed this Dec 22, 2024
@Stephan202
Copy link
Contributor Author

Alright, I pushed a proper fix: it turns out that SlidingWindow#lastRotateTimestampMillis was assigned before the test reassigned the LongSupplier currentTimeMillis. I resolved this by introducing a constructor overload instead.

@Stephan202 Stephan202 reopened this Dec 23, 2024
Fixes issue #956.

Signed-off-by: Stephan Schroevers <stephan.schroevers@teampicnic.com>
@Stephan202 Stephan202 force-pushed the sschroevers/fix-flaky-SlidingWindowTest branch from 3255588 to ceb8941 Compare December 23, 2024 07:39
@Stephan202 Stephan202 changed the title Fix flaky SlidingWindowTest#testRotate Fix flaky SlidingWindowTest Dec 23, 2024
@Stephan202
Copy link
Contributor Author

Stephan202 commented Dec 23, 2024

Squashed the commits and rewrote the PR title and summary.

@dhoard dhoard merged commit 4365604 into prometheus:main Dec 27, 2024
4 checks passed
@Stephan202 Stephan202 deleted the sschroevers/fix-flaky-SlidingWindowTest branch December 27, 2024 06:03
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.

Flaky test - SlidingWindowTest
3 participants