azurite-engine / Azurite

Azurite Game Engine is a 2D Java game engine built on top of LWJGL.
https://azurite-engine.github.io
MIT License
42 stars 20 forks source link

Fix non-determinism in XML element parsing #91

Closed kaiyaok2 closed 1 year ago

kaiyaok2 commented 1 year ago

Description

Currently, XMLElement class uses a HashMap to keep track of the XML attributes. The method toString() then invokes the default iterator of HashMap.entrySet() when parsing the attributes. However, HashMap$EntrySet.iterator eventually calls HashMap$HashIterator, which does not ensure specific iteration order. (From the Javadoc from Oracle: "HashMap class makes no guarantees as to the order of the map; in particular, it does not guarantee that the order will remain constant over time.")

Sample Failure

The HashMap iteration order changed between Java 7 and 8, breaking lots of unit tests. Our NonDex plugin (https://github.com/TestingResearchIllinois/NonDex) permutes these orders just as between Java 7 and 8, and reports a test failure in XMLParserTest:

io.xml.XMLParserTest > parse FAILED
    org.junit.ComparisonFailure: expected:<<root><wangtile [tileid="6" wangid="0,1,0,1,0,1,0,2"/><wangtile tileid="7" wangid="0,2,0,1,0,1,0,1"/><!--wangtile tileid="7" wangid="0,2,0,1,0,1,0,1"/--><wangtile tileid="8" wangid="0,2,0,2,0,2,0,1]"/></root>> but was:<<root><wangtile [wangid="0,1,0,1,0,1,0,2" tileid="6"/><wangtile tileid="7" wangid="0,2,0,1,0,1,0,1"/><!--wangtile tileid="7" wangid="0,2,0,1,0,1,0,1"/--><wangtile wangid="0,2,0,2,0,2,0,1" tileid="8]"/></root>>
        at org.junit.Assert.assertEquals(Assert.java:117)
        at org.junit.Assert.assertEquals(Assert.java:146)
        at io.xml.XMLParserTest.parse(XMLParserTest.java:29)

Fix

Use LinkedHashMap to store the XML attributes. This ensures that the toString() method produces deterministic output.

Juyas commented 1 year ago

Seems reasonable and valid. Thanks for poiting that out - other systems might be impacted by that as well, so we'll be checking that.