Merge pull request #11 from hidakatsuya/fix-loading-xml-file-has-no-linenumber

Fix cannot load properly a XML file containing issue with no lineNumber and errorLine attributes

Author: hidakatsuya

__tests__/check.test.js

@@ -100,31 +100,56 @@ describe("invalid XML file", () => {
   })
 })
 
-test("XML file containing issues with multiple locations", async () => {
-  const results = await check({ pathPattern: path.join(__dirname, "xml/failure3.xml") })
+describe("other XML file formats", () => {
+  test("<issue> with multiple location attributes", async () => {
+    const results = await check({ pathPattern: path.join(__dirname, "xml/failure3.xml") })
 
-  expect(results.status).toEqual("warning")
-  expect(results.failures.length).toEqual(1)
-  expect(results.results.length).toEqual(1)
+    expect(results.status).toEqual("warning")
+    expect(results.failures.length).toEqual(1)
+    expect(results.results.length).toEqual(1)
+
+    const { result } = results.results[0]
+
+    expect(result.status).toEqual("warning")
+    expect(result.issues.length).toEqual(2)
+
+    expect(result.warnings.length).toEqual(2)
+    expect(result.warnings[0].file).toEqual("/path/to/root/app/src/main/res/values/colors1.xml")
+    expect(result.warnings[1].file).toEqual("/path/to/root/app/src/main/res/values/colors2.xml")
+
+    expect(result.errors.length).toEqual(0)
+  })
 
-  const { result } = results.results[0]
+  test("No errorLine and lineNumber attributes", async () => {
+    const results = await check({ pathPattern: path.join(__dirname, "xml/failure4.xml") })
 
-  expect(result.status).toEqual("warning")
-  expect(result.issues.length).toEqual(2)
+    expect(results.status).toEqual("warning")
+    expect(results.failures.length).toEqual(1)
+    expect(results.results.length).toEqual(1)
+
+    const { result } = results.results[0]
+
+    expect(result.status).toEqual("warning")
+    expect(result.issues.length).toEqual(1)
+
+    expect(result.warnings.length).toEqual(1)
+    expect(result.errors.length).toEqual(0)
 
-  expect(result.warnings.length).toEqual(2)
-  expect(result.warnings[0].file).toEqual("/path/to/root/app/src/main/res/values/colors1.xml")
-  expect(result.warnings[1].file).toEqual("/path/to/root/app/src/main/res/values/colors2.xml")
+    const issue = result.issues[0]
 
-  expect(result.errors.length).toEqual(0)
+    expect(issue.id).toEqual("UnusedResources")
+    expect(issue.errorLine1).toBeUndefined()
+    expect(issue.errorLine2).toBeUndefined()
+    expect(issue.lineNumber).toBeUndefined()
+  })
 })
 
 describe("basic glob path pattern", () => {
   test("**/xml/failure*.xml", async () => {
     const results = await check({ pathPattern: "**/xml/failure*.xml" })
 
     expect(results.status).toEqual("error")
-    expect(results.failures.length).toEqual(3)
-    expect(results.results.length).toEqual(3)
+    expect(results.failures.length).toEqual(4)
+    expect(results.results.length).toEqual(4)
   })
 })

__tests__/report.test.js

@@ -4,16 +4,22 @@ const { Issue, Result, Results } = require("../src/check")
 
 const baseDir = "/path/to"
 
-function buildIssue({ severity, identifier = 1 }) {
+function buildIssue({
+  severity,
+  identifier = 1,
+  lineNumber = identifier.toString(),
+  errorLine1 = "line1",
+  errorLine2 = "line2"
+}) {
   return new Issue({
     id: "SomeError",
     file: `/path/to/app/src/source${identifier}.kt`,
-    lineNumber: `${identifier}`,
+    lineNumber,
     severity: severity,
     message: `Error message${identifier}`,
     summary: "Some Error",
-    errorLine1: "line1",
-    errorLine2: "line2"
+    errorLine1,
+    errorLine2
   })
 }
 
@@ -154,3 +160,30 @@ test("multiple results", async () => {
     "</details>"
   ].join("\n"))
 })
+
+describe("other cases", () => {
+  test("no lineNumber and errorLine attribute", async () => {
+    const issue = buildIssue({
+      severity: "Error",
+      identifier: 1,
+      lineNumber: null,
+      errorLine1: null,
+      errorLine2: null
+    })
+    const result = new Result([issue])
+    const results = new Results([{ path: "/path/to/app/build/result.xml", result }])
+
+    await report({ results, core, baseDir })
+
+    const summary = core.summary.stringify()
+
+    expect(summary).toMatch([
+      "<h2>❌ Android Lint</h2>",
+      "<h3>app/build/result.xml</h3>",
+      "<details><summary>❌ Errors</summary>",
+      "",
+      "#### app/src/source1.kt (1 issues)",
+      "* SomeError: Error message1"
+    ].join("\n"))
+  })
+})

__tests__/xml/failure1.xml

@@ -1,4 +1,5 @@
 <?xml version="1.0" encoding="UTF-8"?>
+<!-- contains both warning and errors -->
 <issues format="6" by="lint 8.1.0">
 
     <issue

__tests__/xml/failure2.xml

@@ -1,4 +1,5 @@
 <?xml version="1.0" encoding="UTF-8"?>
+<!-- contains only warning issues  -->
 <issues format="6" by="lint 8.1.0">
 
     <issue

__tests__/xml/failure3.xml

@@ -1,4 +1,5 @@
 <?xml version="1.0" encoding="UTF-8"?>
+<!-- <issue> with multiple location attributes -->
 <issues format="6" by="lint 8.1.0">
 
     <issue

__tests__/xml/failure4.xml

@@ -0,0 +1,17 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!-- no errorLine and lineNumber attributes -->
+<issues format="6" by="lint 8.1.0">
+
+    <issue
+        id="UnusedResources"
+        severity="Warning"
+        message="The resource `R.color.purple_700` appears to be unused"
+        category="Performance"
+        priority="3"
+        summary="Unused resources"
+        explanation="Unused resources make applications larger and slow down builds.&#xA;&#xA;&#xA;The unused resource check can ignore tests. If you want to include resources that are only referenced from tests, consider packaging them in a test source set instead.&#xA;&#xA;You can include test sources in the unused resource check by setting the system property lint.unused-resources.include-tests =true, and to exclude them (usually for performance reasons), use lint.unused-resources.exclude-tests =true.&#xA;,">
+        <location
+            file="/path/to/root/app/src/main/res/values/colors1.xml"/>
+    </issue>
+
+</issues>

dist/index.js

@@ -12,20 +12,35 @@ function buildDetails(issuesEachFile, baseDir) {
     )
 
     issues.forEach(issue => {
-      summaries.push(
-        `* **Line#${issue.lineNumber}** - ${issue.id}: ${issue.message}`,
-        "  ```",
-        `  ${issue.errorLine1}`,
-        `  ${issue.errorLine2}`,
-        "  ```",
-        ""
-      )
+      summaries.push(buildIssueDetail(issue))
     })
   }
 
   return summaries.join("\n")
 }
 
+function buildIssueDetail(issue) {
+  const detail = []
+
+  if (issue.lineNumber) {
+    detail.push(`* **Line#${issue.lineNumber}** - ${issue.id}: ${issue.message}`)
+  } else {
+    detail.push(`* ${issue.id}: ${issue.message}`)
+  }
+
+  if (issue.errorLine1 && issue.errorLine2) {
+    detail.push(
+      "  ```",
+      `  ${issue.errorLine1}`,
+      `  ${issue.errorLine2}`,
+      "  ```"
+    )
+  }
+  detail.push("")
+
+  return detail.join("\n")
+}
+
 function resultIcon(resultStatus) {
   switch (resultStatus) {
     case "error":