[PageImpl] Non-deterministic behavior or HashSet might fail tests in PageImplTest

[mumbler6]

This issue is a copy of #2612 (comment)

Bug Report

Current Behavior

• While testing the PageImpl, the contents of Page are mocked using MockPage from the dependency io.wcm.testing.aem-mock.junit5.
• The mock values for Page is loaded from the following files based on the version of the PageImplTest, where the tags are in the order [one, two, three].
  • src/test/resources/page/test-content.json
  • src/test/resources/page/v2/test-content.json
  • src/test/resources/page/v3/test-content.json

• The mocking occurs after instantiation of PageImpl at
  

  aem-core-wcm-components/bundles/core/src/main/java/com/adobe/cq/wcm/core/components/internal/models/v1/PageImpl.java

  Lines 150 to 159 in a517a37

  	private String[] buildKeywords() {
  	Tag[] tags = currentPage.getTags();
  	String[] keywords = new String[tags.length];
  	int index = 0;
  	Locale language= currentPage.getLanguage(false);
  	for (Tag tag : tags) {
  	keywords[index++] = tag.getTitle(language);
  	}
  	return keywords;
  	}

• However, while mocking the Tags of PageImpl at MockTagManager:380 (screenshots provided below), the values of the tags are converted from the loaded resource into a HashSet.

• The official Javadoc mentions that HashSet does not guarantee that the order will be maintained over time.

• This causes issues where the order of tags of tags is expected to be in a specific order.

• Specifically, the following tests expects the order of the tags after exporting to JSON to be in the order ["three", "one", "two"]

  • com.adobe.cq.wcm.core.components.internal.models.v1.PageImplTest.testPage
  • com.adobe.cq.wcm.core.components.internal.models.v2.PageImplTest.testPage
  • com.adobe.cq.wcm.core.components.internal.models.v2.PageImplTest.testPageWithDeprecatedCaconfig
  • com.adobe.cq.wcm.core.components.internal.models.v3.PageImplTest.testPage
  • com.adobe.cq.wcm.core.components.internal.models.v3.PageImplTest.testPageWithDeprecatedCaconfig

Steps to reproduce

• The above mentioned issue is found using the [NonDex (https://github.com/TestingResearchIllinois/NonDex) tool, which shuffles the order of HashSet.

# Clone the Repo
https://github.com/adobe/aem-core-wcm-components.git

# Compile the module byte-buddy-dep
mvn clean install -pl bundles/core -am -DskipTests

# (Optional) Run the unit test
mvn -pl bundles/core test -Dtest=<Above Mentioned Test Name>

# Run the unit test using NonDex
mvn -pl bundles/core edu.illinois:nondex-maven-plugin:2.1.1:nondex -Dtest=<Above Mentioned Test Name>

• Following the above steps for the mentioned tests will produce the following result:

[INFO] Results:
[INFO] 
[ERROR] Failures: 
[ERROR]   PageImplTest.testPage:109->testPage:142 \
expected: <{"id":"page-bb590de134","designPath":"/etc/designs/mysite","title":"Templated Page","description":"Description","brandSlug":"Brand Slug","lastModifiedDate":1453282416000,"templateName":"product-page","appResourcesPath":"/core/etc.clientlibs/wcm/core/page/clientlibs/favicon/resources","htmlPageItems":[{"element":"link","location":"header","attributes":{"href":"/_theme/theme.css","rel":"preload","as":"style"}},{"element":"script","location":"footer","attributes":{"async":true,"defer":false,"src":"/_theme/theme.js"}},{"element":"meta","location":"header","attributes":{"charset":"UTF-8"}}],"cssClassNames":"class1 class2","language":"en-GB",":itemsOrder":["root"],":items":{"root":{":itemsOrder":["text"],":items":{"text":{"id":"text-41bcd02e64","text":"<p>This is a test</p>","richText":true,":type":"core/wcm/components/text/v2/text","dataLayer":{"text-41bcd02e64":{"@type":"core/wcm/components/text/v2/text","repo:modifyDate":"2016-01-18T15:59:43Z","xdm:text":"<p>This is a test</p>"}}}},":type":"wcm/foundation/components/responsivegrid"}},":type":"core/wcm/components/page/v2/page","dataLayer":{"page-bb590de134":{"@type":"core/wcm/components/page/v2/page","repo:modifyDate":"2016-01-20T09:33:36Z","dc:title":"Templated Page","dc:description":"Description","xdm:template":"/conf/coretest/settings/wcm/templates/product-page","xdm:language":"en-GB","xdm:tags":["three","one","two"],"repo:path":"/core/content/page/templated-page.html"}}}> \
 but was: <{"id":"page-bb590de134","lastModifiedDate":1453282416000,"brandSlug":"Brand Slug","description":"Description","title":"Templated Page","designPath":"/etc/designs/mysite","templateName":"product-page","htmlPageItems":[{"attributes":{"href":"/_theme/theme.css","rel":"preload","as":"style"},"element":"link","location":"header"},{"attributes":{"async":true,"defer":false,"src":"/_theme/theme.js"},"element":"script","location":"footer"},{"attributes":{"charset":"UTF-8"},"element":"meta","location":"header"}],"appResourcesPath":"/core/etc.clientlibs/wcm/core/page/clientlibs/favicon/resources","cssClassNames":"class1 class2","language":"en-GB",":items":{"root":{":itemsOrder":["text"],":items":{"text":{"id":"text-41bcd02e64","text":"<p>This is a test</p>","richText":true,":type":"core/wcm/components/text/v2/text","dataLayer":{"text-41bcd02e64":{"repo:modifyDate":"2016-01-18T15:59:43Z","@type":"core/wcm/components/text/v2/text","xdm:text":"<p>This is a test</p>"}}}},":type":"wcm/foundation/components/responsivegrid"}},":itemsOrder":["root"],":type":"core/wcm/components/page/v2/page","dataLayer":{"page-bb590de134":{"repo:modifyDate":"2016-01-20T09:33:36Z","dc:title":"Templated Page","@type":"core/wcm/components/page/v2/page","dc:description":"Description","xdm:language":"en-GB","xdm:template":"/conf/coretest/settings/wcm/templates/product-page","xdm:tags":["one","three","two"],"repo:path":"/core/content/page/templated-page.html"}}}>
[INFO] 
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0
[INFO] 

• Filtering out the tags from the above log

[INFO] Results:
[INFO] 
[ERROR] Failures: 
[ERROR]   PageImplTest.testPage:109->testPage:142 
expected: ..."xdm:tags":["three","one","two"]... 
  but was: ..."xdm:tags":["one","three","two"]...
[INFO] 
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0
[INFO] 

Expected behavior/code

• The ordering of the tags should have been maintain in a specific order for all runs
• The tests should have passed successfully when run through NonDex

Environment

• AEM version and patch level (e.g. AEM 6.3 SP1 CFP2)
• Core Components version (e.g. 2.0.4)
• JRE version (e.g. Java(TM) SE Runtime Environment (build 1.8.0_152-b16))

Possible Solution

• Even though the order of key value pair could change in the exported JSON, the order of an array needs to be maintained. So, changing the test function to compare based on the presence of elements in an array (like a Set) is incorrect.
• Solution: Sort the ordering of the tags in PageImpl based on the tag name. This ensures that whatever may be order returned by the MockTagManager, we would maintain the same order while exporting to JSON.

+/-  com.adobe.cq.wcm.core.components.internal.models.v1.PageImpl.java:134

    Tag[] tags = currentPage.getTags();
    keywords = new String[tags.length];

    Tag[] tags = currentPage.getTags();
+   Arrays.sort(tags, Comparator.comparing(Tag::getName));
    keywords = new String[tags.length];

• However, this also requires to change the expected order of exported tags from ["three", "one", "two"] to ["one", "three", "two"] because of lexicographic order sorting.
• The ordering of tags needs to changed in the following expected JSON files
  • src/test/resources/page/exporter-templated-page.json
  • src/test/resources/page/v3/exporter-templated-page.json
  • src/test/resources/page/v3/exporter-templated-page.json

      "xdm:tags": [
-       "three",
-       "one",
        "two"
      ]

      "xdm:tags": [
+       "one",
+       "three",
        "two"
      ]

Additional context / Screenshots

• Check the issue linked at the beginning for additional info if needed.