Improve SimpleAliasRegistryTests

[sbrannen]

Overview

This is a followup to #31348.

As stated in the TODO in SimpleAliasRegistryTests, the resolveAliasesWithComplexPlaceholderReplacement() test is flaky.

It passes as-is with ALIAS4 set to "testAlias4", and it also passes for values such as "x", "xx", "xxx", "xxxx", and likely numerous other values.

However, it fails for "alias4", "xxxxx", "testAli", and likely numerous other values.

The issue might be due to the use of a Mockito mock, but I do not currently have the time to investigate it further.

Deliverables

• Determine why the test is flaky.
• Set ALIAS4 to "alias4" once the issue is resolved.

[sbrannen]

@e-freni has volunteered to investigate this issue and potentially submit a PR to resolve it.

[e-freni]

@sbrannen I investigated the issue, and it concerns the hashmap. The length of the string affects the hash used for sorting in the hashmap. Consequently, 'x,' 'xx,' and 'testAlias4' push the key to be the first element tested, which allows the expected exception to be thrown. If you use 'alias4,' the calculated hash pushes the value to the end of the map, thus the method self-corrects the content of 'aliasMap' and prevents the exception from occurring. It can be fixed for 'alias4' if it's fixed, but for all variable string lengths, the test logic needs to be modified to cover those cases. Even though I would make them explicit and not with ALIAS4 constant. Let me know if this solution works for you, and I'll get to it.

[snicoll]

@e-freni thanks very much for investigating. If you're still up for it, we'd be happy to review a PR. Let us know please.

[sbrannen]

Indeed, thanks for the detailed analysis, @e-freni! 👍

The length of the string affects the hash used for sorting in the hashmap. Consequently, 'x,' 'xx,' and 'testAlias4' push the key to be the first element tested, which allows the expected exception to be thrown. If you use 'alias4,' the calculated hash pushes the value to the end of the map, thus the method self-corrects the content of 'aliasMap' and prevents the exception from occurring.

Aha... you're saying the iteration order over the map elements in resolveAliases(...) (via aliasCopy.forEach((alias, registeredName) -> { ...) depends on the hash code values of the alias keys in the map.

So that means the main issue is not with the test but rather what I would consider a bug in SimpleAliasRegistry.

Specifically, changing the names of aliases while maintaining the exact same set of logical alias pairs should not work sometimes and fail at other times.

[sbrannen]

Based on the input from @e-freni and further findings while investigating the issue, I have repurposed this issue to simply improve the status quo for SimpleAliasRegistryTests.

The "flakiness" of the alias resolution algorithm is now being investigated in #32024.