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 flakiness caused by hash map #1133

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

NDR0216
Copy link

@NDR0216 NDR0216 commented Dec 6, 2023

Problem

Running maven tests with NonDex tool would discover flaky tests.
NonDex is a tool for detecting wrong assumptions on undeterministic Java APIs. It could help developers fix the assumption before it becomes a problem far in the future and more difficult to fix.

Reproduce step

  • Test Environment

    Apache Maven: 3.6.3
    Java: 11.0.21
    OS: Ubuntu 20.04.6 LTS
    Linux kernel: 5.4.0-167-generic

mvn edu.illinois:nondex-maven-plugin:2.1.1:nondex

Root cause

EagerTest related test

Take com.hubspot.jinjava.EagerTest#itHandlesImportInDeferredIf as an example, the test fails at assertion in ExpectedTemplateInterpreter.java L45

public String assertExpectedOutputNonIdempotent(String name) {
String template = getFixtureTemplate(name);
String output = JinjavaInterpreter.getCurrent().render(template);
assertThat(JinjavaInterpreter.getCurrent().getContext().getDeferredNodes())
.as("Ensure no deferred nodes were created")
.isEmpty();
assertThat(output.trim()).isEqualTo(expected(name).trim());
return output;

The reason why it fails is that the rendered string is determined by the order of HashMap.entrySet(), which is called at AliasedEagerImportingStrategy.java L255

for (Map.Entry<String, Object> entry : currentAliasMap.entrySet()) {

However, the order of HashMap.entrySet() is not guaranteed. It might change from time to time.
NonDex would shuffle the map on each invocation of methods.
Therefore, the rendered string might change, and make the tests fail.

PyishObjectMapperTest

For PyishObjectMapperTest test, it is the similar reason.
In L25, HashMap has been used. However, the assertion in L29 is relied on the order of HashMap.entrySet()

@Test
public void itSerializesMapWithNullKeysAsEmptyString() {
Map<String, Object> map = new SizeLimitingPyMap(new HashMap<>(), 10);
map.put("foo", "bar");
map.put(null, "null");
assertThat(PyishObjectMapper.getAsPyishString(map))
.isEqualTo("{'': 'null', 'foo': 'bar'} ");
}

Code change

To fix the test flakiness, I replaced HashMap with LinkedHashMap in AliasedEagerImportingStrategy.java and PyishObjectMapperTest.java, and exchange the order of string to aligned with the order of `LinkedHashMap

Failed test

com.hubspot.jinjava.EagerTest#itHandlesImportInDeferredIf
com.hubspot.jinjava.EagerTest#itHandlesDoubleImportModification
com.hubspot.jinjava.EagerTest#itHandlesSameNameImportVar
com.hubspot.jinjava.EagerTest#itDoesNotOverrideImportModificationInFor
com.hubspot.jinjava.EagerTest#itRreconstructsValueUsedInDeferredImportedMacro
com.hubspot.jinjava.EagerTest#itAllowsVariableSharingAliasName
com.hubspot.jinjava.NonRevertingEagerTest#itHandlesImportInDeferredIf
com.hubspot.jinjava.NonRevertingEagerTest#itHandlesDoubleImportModification
com.hubspot.jinjava.NonRevertingEagerTest#itHandlesSameNameImportVar
com.hubspot.jinjava.NonRevertingEagerTest#itDoesNotOverrideImportModificationInFor
com.hubspot.jinjava.NonRevertingEagerTest#itRreconstructsValueUsedInDeferredImportedMacro
com.hubspot.jinjava.NonRevertingEagerTest#itAllowsVariableSharingAliasName

com.hubspot.jinjava.lib.tag.eager.EagerImportTagTest#itDefersTripleLayer
com.hubspot.jinjava.lib.tag.eager.EagerImportTagTest#itHandlesQuadLayerInDeferredIf
com.hubspot.jinjava.lib.tag.eager.EagerImportTagTest#itDoesNotSilentlyOverrideVariable

com.hubspot.jinjava.objects.serialization.PyishObjectMapperTest#itSerializesMapWithNullKeysAsEmptyString
com.hubspot.jinjava.objects.serialization.PyishObjectMapperTest#itSerializesMapEntrySet
com.hubspot.jinjava.objects.serialization.PyishObjectMapperTest#itSerializesMapEntrySetWithLimit
com.hubspot.jinjava.objects.serialization.PyishObjectMapperTest#itSerializesMapWithNullValues

@NDR0216
Copy link
Author

NDR0216 commented Dec 7, 2023

Similar reason to #827

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.

1 participant