Fix flaky test testExternalUrl using LinkedHashMap

[KiruthikaJanakiraman]

Checklist

• the individual contributor license agreement is signed
• commit message follows commit guidelines

Description of change

This PR aims to fix the flaky test: org.apereo.portal.redirect.PortletRedirectionControllerTest.testExternalUrl similar to a previously raised PR #2718.

I found and confirmed the flaky behaviour of the tests using an open-source research tool NonDex, which shuffles implementations of nondeterminism operations.

Problem
The test case org.apereo.portal.redirect.PortletRedirectionControllerTest.testExternalUrl fails due to the below assertion.

uPortal/uPortal-webapp/src/test/java/org/apereo/portal/redirect/PortletRedirectionControllerTest.java

Lines 57 to 60 in 989ba28

	String expected =
	"http://somewhere.com/something?action=show&list=v1&list=v2&username=student";
	String actual = controller.getUrlString(url, request, new ArrayList<String>());
	assertEquals(expected, actual);

This is because additionalParameters in url is a HashMap which does not preserve the order of the items causing the order of action and list to vary.

uPortal/uPortal-webapp/src/test/java/org/apereo/portal/redirect/PortletRedirectionControllerTest.java

Line 48 in 989ba28

Map<String, String[]> additionalParameters = new HashMap<String, String[]>();

Solution
This PR fixes the test by using a custom function compareQueryParameters to compare two urls irrespective of the order of the query parameters.

Steps to reproduce
The following command can be used to reproduce the assertion errors and verify the fix.

Integrate NonDex:

Add the following snippet to the top of the build.gradle in $PROJ_DIR:

plugins {
    id 'edu.illinois.nondex' version '2.1.1-1'
}

Append to build.gradle in $PROJ_DIR:

apply plugin: 'edu.illinois.nondex'

Execute Test with Gradle:

./gradlew --info uPortal-webapp:test --tests org.apereo.portal.redirect.PortletRedirectionControllerTest.testExternalUrl

Run NonDex:

./gradlew --info uPortal-webapp:nondexTest --tests=org.apereo.portal.redirect.PortletRedirectionControllerTest.testExternalUrl --nondexRuns=50 -x autoLintGradle

Test Environment:

Java version "1.8.0_381"
macOS Venture Version 13.4.1 (22F82)

Please let me know if you have any concerns or questions.

[ChristianMurphy]

Thanks @KiruthikaJanakiraman!
To clarify, I believe the intent of this test is to be order insensitive.
Would this change it to become order sensitive and potentially cause tests to fail over a property (order) we don't intend to check?

[KiruthikaJanakiraman]

Hi,
The assert statement checks for a specific order of parameters. Doesn't that make the test order sensitive?
Since the order of iteration is not guaranteed in a HashMap, the order of the parameters tends to get shuffled, causing the test to fail as the test is checking for a specific url string.
By using a LinkedHashMap, I am trying to preserve the order of the parameters.

If the test is meant to be order insensitive, an alternative fix could be to assert for both possible orders, i.e.,
"http://somewhere.com/something?action=show&list=v1&list=v2&username=student"
OR
"http://somewhere.com/something?list=v1&list=v2&action=show&username=student"

If you prefer the latter fix, kindly let me know and I could update the PR.

[ChristianMurphy]

The assert statement checks for a specific order of parameters. Doesn't that make the test order sensitive?

You are right, the test in is current form is indeed order sensitive.
This is testing URL query params. Query params are not order sensitive.
I'd personally lean that the test could/should account query params being in any/different order.
But I'd be interested to hear what you (@KiruthikaJanakiraman) and others (@uPortal-Project/uportal-committers) think.

[bjagg]

I think the test should be changed to account for any order.

[KiruthikaJanakiraman]

This is testing URL query params. Query params are not order sensitive.

That makes sense. If the test is meant to be order insensitive, I would suggest 2 alternative fixes:

1. Checking for both possible order of additionalParameters
2. Writing a custom function to compare urls with different query parameter orders

Could you kindly let me know the preferred fix?

[KiruthikaJanakiraman]

I have updated the commit with a custom function that compares two urls two urls irrespective of the order of the query parameters. Could you kindly review the same?

[ChristianMurphy]

Thanks @KiruthikaJanakiraman!

[ChristianMurphy]

@KiruthikaJanakiraman could you run ./gradlew goJF to format the changes?
CI is getting stuck on the format checking step https://github.com/uPortal-Project/uPortal/actions/runs/6828454623/job/18572610313?pr=2719#step:5:51