Skip to content
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.

Sign up for GitHub

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

Merged
merged 22 commits into from
May 8, 2020
Merged

#764 Fix crash on parse some control chars #771

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
f182c8c
#764
adamfilipow92 May 7, 2020
6be0d1f
#764 detekt issues fix
adamfilipow92 May 7, 2020
10a1799
Asserts in tests
adamfilipow92 May 7, 2020
9387a48
Add documentation of magicial numers
adamfilipow92 May 7, 2020
ee575ae
set name of UtfControlChars enum to UtfControlCharRanges
adamfilipow92 May 7, 2020
b36c3b5
#764
adamfilipow92 May 7, 2020
9b025ac
#764 detekt issues fix
adamfilipow92 May 7, 2020
77b574b
Asserts in tests
adamfilipow92 May 7, 2020
1ba4a77
Add documentation of magicial numers
adamfilipow92 May 7, 2020
795f9e2
set name of UtfControlChars enum to UtfControlCharRanges
adamfilipow92 May 7, 2020
58074c3
Detekt suggestions
adamfilipow92 May 7, 2020
4a484aa
Create should_exists.txt
adamfilipow92 May 7, 2020
9a6f328
Merge branch 'bug/#764' of https://github.com/Flank/flank into bug/#764
adamfilipow92 May 7, 2020
2e9f925
Fail fast when results-dir is incorrect (#772)
jan-goral May 7, 2020
da1fa2a
#764
adamfilipow92 May 7, 2020
dfd80fb
#764 detekt issues fix
adamfilipow92 May 7, 2020
793b97e
Asserts in tests
adamfilipow92 May 7, 2020
6f8fc9e
Add documentation of magicial numers
adamfilipow92 May 7, 2020
5350fc3
set name of UtfControlChars enum to UtfControlCharRanges
adamfilipow92 May 7, 2020
df19d79
#764 Change XmlPreprocessor more functional and
adamfilipow92 May 8, 2020
6c60453
Add info about issue to release notes
adamfilipow92 May 8, 2020
e757e02
Merge branch 'master' into bug/#764
adamfilipow92 May 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions test_runner/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,8 @@ dependencies {
implementation(Libs.SYSTEM_RULES)
testImplementation(Libs.TRUTH)
testImplementation(Libs.MOCKK)

implementation(Libs.COMMON_TEXT)
}

// Fix Exception in thread "main" java.lang.NoSuchMethodError: com.google.common.hash.Hashing.crc32c()Lcom/google/common/hash/HashFunction;
Expand Down
4 changes: 4 additions & 0 deletions test_runner/buildSrc/src/main/kotlin/Deps.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ object Versions {

// https://github.com/mockk/mockk
const val MOCKK = "1.9.3"

const val COMMON_TEXT = "1.7"
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment added

}

object Libs {
Expand Down Expand Up @@ -123,4 +125,6 @@ object Libs {
const val TRUTH = "com.google.truth:truth:${Versions.TRUTH}"
const val MOCKK = "io.mockk:mockk:${Versions.MOCKK}"
//endregion

const val COMMON_TEXT = "org.apache.commons:commons-text:${Versions.COMMON_TEXT}"
}
24 changes: 9 additions & 15 deletions test_runner/src/main/kotlin/ftl/reports/xml/JUnitXml.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 Files.readString(path)
}

fun JUnitTestResult?.xmlToString(): String {
Expand All @@ -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)
}
14 changes: 14 additions & 0 deletions test_runner/src/main/kotlin/ftl/reports/xml/preprocesor/XmlPreprocessor.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package ftl.reports.xml.preprocesor

import org.apache.commons.text.StringEscapeUtils

fun fixHtmlCodes(data: String): String {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO expressions > blocks ;)

fun fixHtmlCodes(data: String): String = listOf(
    UtfControlCharRanges.CONTROL_TOP_START.charValue..UtfControlCharRanges.CONTROL_TOP_END.charValue,
    UtfControlCharRanges.CONTROL_BOTTOM_START.charValue..UtfControlCharRanges.CONTROL_BOTTOM_END.charValue
).flatten()
    .map { StringEscapeUtils.escapeXml11(it.toChar().toString()) }
    .filter { it.startsWith("&#") }
    .fold(data) { fixedStr: String, isoControlCode: String -> fixedStr.replace(isoControlCode, "") }

unions are not mandatory for those values

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am also for @jan-gogo version 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Changed to better version :)

val isoHtmlCodesToReplace = listOf(0x00..0x1F).union(listOf(0x7F..0x9F)).flatten()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some document we can reference in a comment about where these magic numbers come from?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other thought I had is, are we sure there's no helper already defined in a library that does what we need? Maybe this is a special case so the custom code is justified.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I this case we need to get list of control chars and convert them to html codes with avoid white chars. I cant find method doing this.
About magic numbers: Im added enum with I hope intuitive names. Enum on the top has comment about destination and link to utf-8 table to prove my idea.

.map { StringEscapeUtils.escapeXml11(it.toChar().toString()) }.filter { it.startsWith("&#") }

var fixedStr = data
for (isoControlCode in isoHtmlCodesToReplace) {
fixedStr = fixedStr.replace(isoControlCode, "")
}
return fixedStr
}
37 changes: 34 additions & 3 deletions test_runner/src/test/kotlin/ftl/reports/xml/JUnitXmlTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,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>
Expand All @@ -94,7 +95,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' ?>
Expand Down Expand Up @@ -423,7 +425,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>
Expand All @@ -437,4 +440,32 @@ junit.framework.Assert.fail(Assert.java:50)</failure>
""".trimIndent()
assertThat(merged).isEqualTo(expected)
}
@Test
fun `parse ftl quirks`() {
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 &#8;&#8;&#8;(Coroutine boundary.&#8;(&#8;)</failure>
</testcase>
<testcase name="b()" classname="b" time="2.0"/>
<testcase name="c()" classname="c" time="7.0"/>
</testsuite>
</testsuites>
""".trimIndent()
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 &#8;&#8;&#8;(Coroutine boundary.&#8;(&#8;)</failure>
</testcase>
<testcase name="b()" classname="b" time="2.0"/>
<testcase name="c()" classname="c" time="7.0"/>
</testsuite>
""".trimIndent()

parseAllSuitesXml(crashingAllSuitesMessage)
parseOneSuiteXml(crashingOneSuiteMessage)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we also assert that the parsed value matches the expected value? Currently this test will pass as long as parsing doesn't fail. We probably want to assert <failure>... is decoded correctly as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have right, Im add asserts and split tests to two. One for one suite and one for all suites

}
}