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 Test - SetSectionTest#testLiterals #43820

Closed

Conversation

mohitbadve
Copy link

@mohitbadve mohitbadve commented Oct 10, 2024

This PR addresses an issue with the test testLiterals() in the Qute engine that was intermittently failing due to non-deterministic evaluation order of template literals. The original test assumed synchronous execution of the template rendering, which led to an assertEquals failure because the variables alpha and baz were being rendered in reverse order (e.g., "1::4::false::Andy" instead of the expected "1::4::Andy::false").

Changes Made:

  • Replaced the synchronous render() call with the asynchronous renderAsync() to ensure that the template is fully processed before comparing the output.
  • Added a .thenAccept() handler to perform the assertEquals check after the asynchronous rendering is complete, ensuring the correct order of variable evaluation and eliminating flakiness.

@mkouba
Copy link
Contributor

mkouba commented Oct 14, 2024

Hi @mohitbadve, thanks for the report. Did you actually observe an intermittent failure with this test locally or elsewhere?

I took a look at the test again and I don't think there's anything wrong. First of all, synchronous or asynchronous execution should not really matter because both methods call the same (async) logic under the hood. The render() method blocks the current thread and waits for the result though. Moreover, (1) the params of a set/let sections are always named, and (2) the section is always executed after all params are resolved.

@mohitbadve
Copy link
Author

Hello @mkouba, thanks for your response.

You have raised valid points about the logic under the hood.

I used NonDex to detect the flakiness of the test. It can be used to detect nondeterministic behavior of tests under certain conditions which usually don't get considered in the local environment.

To verify the issue, you can run NonDex via Maven as follows:
mvn edu.illinois:nondex-maven-plugin:2.1.7:nondex -Dtest=io.quarkus.qute.SetSectionTest#testLiterals -DnondexRuns=10

While figuring out the solution, I thought that the call to render() blocks the current thread, which may not always yield consistent results if there are timing issues. By using renderAsync(), you allow the rendering to complete without blocking the thread. While I'm not entirely sure about the exact reason for the flakiness, switching to asynchronous rendering seems to solve the issue.

Looking forward to discussing this further.

@mkouba
Copy link
Contributor

mkouba commented Oct 15, 2024

Hello @mkouba, thanks for your response.

You have raised valid points about the logic under the hood.

I used NonDex to detect the flakiness of the test. It can be used to detect nondeterministic behavior of tests under certain conditions which usually don't get considered in the local environment.

I've never heard of this tool but it seems that its main feature is to shuffle the ordering for the supported APIs.

Unfortunately, the debug results do not seem to be very helpful:

TEST: io.quarkus.qute.SetSectionTest#testLiterals
java.base/java.lang.Thread.getStackTrace(Thread.java:1619)
java.base/edu.illinois.nondex.common.NonDex.printStackTraceIfUniqueDebugPoint(NonDex.java:165)
java.base/edu.illinois.nondex.common.NonDex.shouldExplore(NonDex.java:136)
java.base/edu.illinois.nondex.common.NonDex.getPermutation(NonDex.java:106)
java.base/edu.illinois.nondex.shuffling.ControlNondeterminism.shuffle(ControlNondeterminism.java:74)
java.base/java.util.HashMap$HashIterator$HashIteratorShuffler.<init>(Unknown Source)
java.base/java.util.HashMap$HashIterator.<init>(HashMap.java:1587)
java.base/java.util.HashMap$EntryIterator.<init>(HashMap.java:1628)
java.base/java.util.HashMap$EntrySet.iterator(HashMap.java:1098)
io.quarkus.qute.Futures.evaluateParams(Futures.java:48)
io.quarkus.qute.SectionNode$SectionResolutionContextImpl.evaluate(SectionNode.java:229)
io.quarkus.qute.SetSectionHelper.resolve(SetSectionHelper.java:42)

Actually, I added some logging in the code and I think that I found the problem. And it's not in the test but in the way the params of the set section are evaluated. We expect the iterator of a HashMap#entrySet() to be consistent (until modifications are made to the map). This is obviously not guaranteed, although in practice it usually holds true (at least we did not observe problems yet).

I will send a follow-up pull request with a fix.

To verify the issue, you can run NonDex via Maven as follows: mvn edu.illinois:nondex-maven-plugin:2.1.7:nondex -Dtest=io.quarkus.qute.SetSectionTest#testLiterals -DnondexRuns=10

While figuring out the solution, I thought that the call to render() blocks the current thread, which may not always yield consistent results if there are timing issues. By using renderAsync(), you allow the rendering to complete without blocking the thread. While I'm not entirely sure about the exact reason for the flakiness, switching to asynchronous rendering seems to solve the issue.

Looking forward to discussing this further.

mkouba added a commit to mkouba/quarkus that referenced this pull request Oct 15, 2024
- iterators of a HashMap#entrySet() are not guaranteed to be consistent;
even if no modifications are made to the map
- this issue was discovered by @mohitbadve with the
https://github.com/TestingResearchIllinois/NonDex project
- see also quarkusio#43820
@mkouba
Copy link
Contributor

mkouba commented Oct 15, 2024

Superseded by #43875.

@mkouba mkouba closed this Oct 15, 2024
@quarkus-bot quarkus-bot bot added the triage/invalid This doesn't seem right label Oct 15, 2024
@mohitbadve
Copy link
Author

Thank you so much for taking the time to thoroughly understand the issue and for considering my PR. I really appreciate the effort you put into analyzing the problem and coming up with a solution. Your changes make sense, and I’m glad that they have been merged.

@mkouba
Copy link
Contributor

mkouba commented Oct 16, 2024

Thank you so much for taking the time to thoroughly understand the issue and for considering my PR. I really appreciate the effort you put into analyzing the problem and coming up with a solution. Your changes make sense, and I’m glad that they have been merged.

Thank you for the report! ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage/invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants