fixed flaky test. The issue is the test method was directly comparing… #3
Conversation
… the string versions of XML files, which included differences in attribute order. Fixed by Enhanced the XML comparison algorithm to address flakiness. The updated method now correctly compares XML files for equivalence, disregarding variations in attribute order.
There was a problem hiding this comment.
Beyond the problems that I pointed out in the code changes, could you also improve the PR description to better match the following (i.e., same headers and the respective content for your PR under each header)?
wildfly/wildfly-core#5366
| @@ -147,7 +153,7 @@ protected String convertFileToString(File file) throws IOException { | |||
| if (line.contains("</lastmod>")) { | |||
| continue; | |||
| } | |||
| line = line.replaceAll("\\s+", ""); | |||
| line = line.replaceAll("\\s+", " "); | |||
There was a problem hiding this comment.
Is this change necessary to fix the flaky test?
There was a problem hiding this comment.
Yes it is required, XML files are compared based on their structure.
common/pom.xml
Outdated
| @@ -44,6 +44,11 @@ | |||
| </execution> | |||
| </executions> | |||
| </plugin> | |||
| <plugin> | |||
| <groupId>edu.illinois</groupId> | |||
| <artifactId>nondex-maven-plugin</artifactId> | |||
There was a problem hiding this comment.
Please remove the change to add NonDex in your pull request. Developers are unlikely to add NonDex to their list of dependencies.
common/pom.xml
Outdated
| @@ -144,6 +149,11 @@ | |||
| </exclusion> | |||
| </exclusions> | |||
| </dependency> | |||
| <dependency> | |||
| <groupId>org.xmlunit</groupId> | |||
| <artifactId>xmlunit-matchers</artifactId> | |||
There was a problem hiding this comment.
Could you accomplish the fix without adding a new dependency to the project?
There was a problem hiding this comment.
I have fixed the issue without using the dependency. Pls review
|
Hey Professor @winglam |
There was a problem hiding this comment.
I'm not sure if using the DocumentBuilder is really the most succinct way to fix this test as doing so may make this method/test much more expensive.
More importantly, it seems the changes still contain many extraneous lines/changes. Please review each changed line of the pull request and confirm whether you actually need to make the change or not and whether that line can be improved or not.
| import javax.xml.XMLConstants; | ||
| import javax.xml.parsers.DocumentBuilder; | ||
| import javax.xml.parsers.DocumentBuilderFactory; | ||
| import java.io.*; |
There was a problem hiding this comment.
Please sort your imports and do NOT use *, instead import only what you need.
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| import static org.hamcrest.MatcherAssert.assertThat; |
| doc2.normalizeDocument(); | ||
| Assert.assertTrue(doc1.isEqualNode(doc2)); | ||
| } catch (Exception e) { | ||
| throw new IOException(); |
There was a problem hiding this comment.
What you are doing here will mask what the actual exception is. Instead of catching all Exceptions and converting them to IOException, please just throw the actual exception in the method signature.
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| import static org.hamcrest.MatcherAssert.assertThat; | ||
| import static org.xmlunit.matchers.CompareMatcher.isSimilarTo; |
What is the purpose of this PR
org.broadleafcommerce.common.sitemap.service.CustomUrlSiteMapGeneratorTest.testCustomUrlSiteMapGeneratorWhy the tests fail
The issue is the test method was directly comparing the string versions of XML files, which included differences in attribute order.
Reproduce the test failure
Run the tests with NonDex maven plugin. The commands to recreate the flaky test failures are:
mvn -pl common edu.illinois:nondex-maven-plugin:2.1.1:nondex -Dtest=org.broadleafcommerce.common.sitemap.service.CustomUrlSiteMapGeneratorTest#testCustomUrlSiteMapGeneratorExpected results
Actual Result
mvn -pl common edu.illinois:nondex-maven-plugin:2.1.1:nondex -Dtest=org.broadleafcommerce.common.sitemap.service.CustomUrlSiteMapGeneratorTest#testCustomUrlSiteMapGenerator[ERROR]
testCustomUrlSiteMapGenerator(org.broadleafcommerce.common.sitemap.service.CustomUrlSiteMapGeneratorTest)Time elapsed: 0.825 s <<< FAILURE!java.lang.AssertionErrorat
org.broadleafcommerce.common.sitemap.service.CustomUrlSiteMapGeneratorTest.testCustomUrlSiteMapGenerator(CustomUrlSiteMapGeneratorTest.java:61)Fix
DocumentBuilderFactoryand Various features of the DocumentBuilderFactory are set to ensure secure and efficient XML processing. Converted the contents of both the files to strings. Two Document objects (doc1 and doc2) are created by parsing the respective XML strings using theDocumentBuilderobtainesd from the DocumentBuilderFactory. Both documents are normalized using thenormalizeDocument()method. This helps ensure consistent comparison by removing any extraneous whitespace or unnecessary structure differences. And compared the both docs usingisEqulaNode()method.