-
Notifications
You must be signed in to change notification settings - Fork 119
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#764 Fix crash on parse some control chars #771
Changes from 5 commits
f182c8c
6be0d1f
10a1799
9387a48
ee575ae
b36c3b5
9b025ac
77b574b
1ba4a77
795f9e2
58074c3
4a484aa
9a6f328
2e9f925
da1fa2a
dfd80fb
793b97e
6f8fc9e
5350fc3
df19d79
6c60453
e757e02
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,20 +6,22 @@ import com.fasterxml.jackson.module.kotlin.KotlinModule | |
import com.fasterxml.jackson.databind.DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES | ||
import ftl.reports.xml.model.JUnitTestResult | ||
import ftl.reports.xml.model.JUnitTestSuite | ||
import ftl.reports.xml.preprocesor.fixHtmlCodes | ||
import java.io.File | ||
import java.nio.file.Files | ||
import java.nio.file.Path | ||
|
||
private val xmlModule = JacksonXmlModule().apply { setDefaultUseWrapper(false) } | ||
|
||
private val xmlMapper = XmlMapper(xmlModule) | ||
.registerModules(KotlinModule()) | ||
.configure(FAIL_ON_UNKNOWN_PROPERTIES, false) | ||
|
||
internal val xmlPrettyWriter = xmlMapper.writerWithDefaultPrettyPrinter() | ||
|
||
private fun xmlBytes(path: Path): ByteArray { | ||
private fun xmlText(path: Path): String { | ||
if (!path.toFile().exists()) throw RuntimeException("$path doesn't exist!") | ||
return Files.readAllBytes(path) | ||
return String(Files.readAllBytes(path)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd like to merge #767 before merging this pull request. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Done |
||
} | ||
|
||
fun JUnitTestResult?.xmlToString(): String { | ||
|
@@ -28,36 +30,28 @@ fun JUnitTestResult?.xmlToString(): String { | |
return prefix + xmlPrettyWriter.writeValueAsString(this) | ||
} | ||
|
||
fun parseOneSuiteXml(bytes: ByteArray): JUnitTestResult { | ||
return JUnitTestResult(mutableListOf(xmlMapper.readValue(bytes, JUnitTestSuite::class.java))) | ||
} | ||
|
||
fun parseOneSuiteXml(path: Path): JUnitTestResult { | ||
return parseOneSuiteXml(xmlBytes(path)) | ||
return parseOneSuiteXml(xmlText(path)) | ||
} | ||
|
||
fun parseOneSuiteXml(path: File): JUnitTestResult { | ||
return parseOneSuiteXml(xmlBytes(path.toPath())) | ||
return parseOneSuiteXml(xmlText(path.toPath())) | ||
} | ||
|
||
fun parseOneSuiteXml(data: String): JUnitTestResult { | ||
return parseOneSuiteXml(data.toByteArray()) | ||
return JUnitTestResult(mutableListOf(xmlMapper.readValue(fixHtmlCodes(data), JUnitTestSuite::class.java))) | ||
} | ||
|
||
// -- | ||
|
||
fun parseAllSuitesXml(bytes: ByteArray): JUnitTestResult { | ||
return xmlMapper.readValue(bytes, JUnitTestResult::class.java) | ||
} | ||
|
||
fun parseAllSuitesXml(path: Path): JUnitTestResult { | ||
return parseAllSuitesXml(xmlBytes(path)) | ||
return parseAllSuitesXml(xmlText(path)) | ||
} | ||
|
||
fun parseAllSuitesXml(path: File): JUnitTestResult { | ||
return parseAllSuitesXml(path.toPath()) | ||
} | ||
|
||
fun parseAllSuitesXml(data: String): JUnitTestResult { | ||
return parseAllSuitesXml(data.toByteArray()) | ||
return xmlMapper.readValue(fixHtmlCodes(data), JUnitTestResult::class.java) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
package ftl.reports.xml.preprocesor | ||
|
||
/** | ||
* Numbers come from ascii table https://www.utf8-chartable.de. | ||
* and represents control chars. We need to avoid characters in ranges CONTROL_TOP_START..CONTROL_TOP_END and | ||
* CONTROL_BOTTOM_START..CONTROL_BOTTOM_END because chars from that range escaped to html causing parsing errors. | ||
* */ | ||
enum class UtfControlCharRanges(val charValue: Int) { | ||
CONTROL_TOP_START(0x00), | ||
CONTROL_TOP_END(0x1F), | ||
CONTROL_BOTTOM_START(0x7F), | ||
CONTROL_BOTTOM_END(0x9F) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
package ftl.reports.xml.preprocesor | ||
|
||
import org.apache.commons.text.StringEscapeUtils | ||
|
||
fun fixHtmlCodes(data: String): String { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO expressions > blocks ;)
unions are not mandatory for those values There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am also for @jan-gogo version 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! Changed to better version :) |
||
val isoHtmlCodesToReplace = | ||
listOf(UtfControlCharRanges.CONTROL_TOP_START.charValue..UtfControlCharRanges.CONTROL_TOP_END.charValue).union( | ||
listOf(UtfControlCharRanges.CONTROL_BOTTOM_START.charValue..UtfControlCharRanges.CONTROL_BOTTOM_END.charValue) | ||
).flatten() | ||
.map { StringEscapeUtils.escapeXml11(it.toChar().toString()) }.filter { it.startsWith("&#") } | ||
|
||
var fixedStr = data | ||
for (isoControlCode in isoHtmlCodesToReplace) { | ||
fixedStr = fixedStr.replace(isoControlCode, "") | ||
} | ||
return fixedStr | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ package ftl.reports.xml | |
|
||
import com.google.common.truth.Truth.assertThat | ||
import ftl.test.util.TestHelper.normalizeLineEnding | ||
import org.junit.Assert | ||
import java.nio.file.Paths | ||
import org.junit.Test | ||
|
||
|
@@ -73,7 +74,8 @@ junit.framework.Assert.fail(Assert.java:50)</failure> | |
|
||
@Test | ||
fun `merge ios`() { | ||
val merged = parseAllSuitesXml(iosPassXml).merge(parseAllSuitesXml(iosFailXml)).xmlToString().normalizeLineEnding() | ||
val merged = | ||
parseAllSuitesXml(iosPassXml).merge(parseAllSuitesXml(iosFailXml)).xmlToString().normalizeLineEnding() | ||
val expected = """ | ||
<?xml version='1.0' encoding='UTF-8' ?> | ||
<testsuites> | ||
|
@@ -94,7 +96,8 @@ junit.framework.Assert.fail(Assert.java:50)</failure> | |
|
||
@Test | ||
fun `Merge iOS large time`() { | ||
val merged = parseAllSuitesXml(iosLargeNum).merge(parseAllSuitesXml(iosLargeNum)).xmlToString().normalizeLineEnding() | ||
val merged = | ||
parseAllSuitesXml(iosLargeNum).merge(parseAllSuitesXml(iosLargeNum)).xmlToString().normalizeLineEnding() | ||
|
||
val expected = """ | ||
<?xml version='1.0' encoding='UTF-8' ?> | ||
|
@@ -423,7 +426,8 @@ junit.framework.Assert.fail(Assert.java:50)</failure> | |
// * c() failed in newRun and passed in oldRun. timing info copied over from oldRun | ||
// * d() was skipped in newRun and successful in oldRun. d() is excluded from the merged result | ||
|
||
val merged = parseAllSuitesXml(newRun).mergeTestTimes(parseAllSuitesXml(oldRun)).xmlToString().normalizeLineEnding() | ||
val merged = | ||
parseAllSuitesXml(newRun).mergeTestTimes(parseAllSuitesXml(oldRun)).xmlToString().normalizeLineEnding() | ||
val expected = """ | ||
<?xml version='1.0' encoding='UTF-8' ?> | ||
<testsuites> | ||
|
@@ -437,4 +441,64 @@ junit.framework.Assert.fail(Assert.java:50)</failure> | |
""".trimIndent() | ||
assertThat(merged).isEqualTo(expected) | ||
} | ||
|
||
@Test | ||
fun `parse ftl quirks in all suites`() { | ||
val crashingAllSuitesMessage = """ | ||
<?xml version='1.0' encoding='UTF-8' ?> | ||
<testsuites> | ||
<testsuite name="EarlGreyExampleSwiftTests" tests="3" failures="0" errors="0" skipped="0" time="10.0" hostname="localhost"> | ||
<testcase name="a()" classname="a" time="1.0"> | ||
<failure> java.net.ConnectException: Failed to connect to ... at (Coroutine boundary.()</failure> | ||
</testcase> | ||
<testcase name="b()" classname="b" time="2.0"/> | ||
<testcase name="c()" classname="c" time="7.0"/> | ||
</testsuite> | ||
</testsuites> | ||
""".trimIndent() | ||
|
||
val expectedAllSuitesMessage = """ | ||
<?xml version='1.0' encoding='UTF-8' ?> | ||
<testsuites> | ||
<testsuite name="EarlGreyExampleSwiftTests" tests="3" failures="0" errors="0" skipped="0" time="10.0" hostname="localhost"> | ||
<testcase name="a()" classname="a" time="1.0"> | ||
<failure> java.net.ConnectException: Failed to connect to ... at (Coroutine boundary.()</failure> | ||
</testcase> | ||
<testcase name="b()" classname="b" time="2.0"/> | ||
<testcase name="c()" classname="c" time="7.0"/> | ||
</testsuite> | ||
</testsuites> | ||
""".trimIndent() | ||
val allSuitesXml = parseAllSuitesXml(crashingAllSuitesMessage).xmlToString().trimIndent() | ||
Assert.assertEquals("All Suite Messages should be the same!", expectedAllSuitesMessage, allSuitesXml) | ||
} | ||
|
||
@Test | ||
fun `parse ftl quirks in on suite`() { | ||
val crashingOneSuiteMessage = """ | ||
<?xml version='1.0' encoding='UTF-8' ?> | ||
<testsuite name="EarlGreyExampleSwiftTests" tests="3" failures="0" errors="0" skipped="0" time="10.0" hostname="localhost"> | ||
<testcase name="a()" classname="a" time="1.0"> | ||
<failure> java.net.ConnectException: Failed to connect to ... at (Coroutine boundary.()</failure> | ||
</testcase> | ||
<testcase name="b()" classname="b" time="2.0"/> | ||
<testcase name="c()" classname="c" time="7.0"/> | ||
</testsuite> | ||
""".trimIndent() | ||
|
||
val expectedOneSuiteMessage = """ | ||
<?xml version='1.0' encoding='UTF-8' ?> | ||
<testsuites> | ||
<testsuite name="EarlGreyExampleSwiftTests" tests="3" failures="0" errors="0" skipped="0" time="10.0" hostname="localhost"> | ||
<testcase name="a()" classname="a" time="1.0"> | ||
<failure> java.net.ConnectException: Failed to connect to ... at (Coroutine boundary.()</failure> | ||
</testcase> | ||
<testcase name="b()" classname="b" time="2.0"/> | ||
<testcase name="c()" classname="c" time="7.0"/> | ||
</testsuite> | ||
</testsuites> | ||
""".trimIndent() | ||
val oneSuiteXml = parseOneSuiteXml(crashingOneSuiteMessage).xmlToString().trimIndent() | ||
Assert.assertEquals("One Suite Messages should be the same!", expectedOneSuiteMessage, oneSuiteXml) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The tests look great! Thanks for updating these. |
||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, add here link to repo or equivalent, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment added