Improve path normalization

lib/reporter.js

@@ -15,6 +15,7 @@ export default class Reporter {
     this.request = params.request || window.fetch
     this.alwaysReport = params.hasOwnProperty('alwaysReport') ? params.alwaysReport : false
     this.reportPreviousErrors = params.hasOwnProperty('reportPreviousErrors') ? params.reportPreviousErrors : true
+    this.resourcePath = this.normalizePath(params.resourcePath || process.resourcesPath)
     this.reportedErrors = []
     this.reportedAssertionFailures = []
   }
@@ -57,7 +58,7 @@ export default class Reporter {
   buildStackTraceJSON (error, projectRoot) {
     return this.parseStackTrace(error).map(callSite => {
       return {
-        file: this.normalizePath(callSite.getFileName()),
+        file: this.scrubPath(callSite.getFileName()),
         method: callSite.getMethodName() || callSite.getFunctionName() || "none",
         lineNumber: callSite.getLineNumber(),
         columnNumber: callSite.getColumnNumber(),
@@ -66,12 +67,22 @@ export default class Reporter {
     })
   }
 
-  normalizePath (path) {
-    return path.replace('file:///', '')                         // Randomly inserted file url protocols
-               .replace(/[/]/g, '\\')                           // Temp switch for Windows home matching
-               .replace(fs.getHomeDirectory(), '~')             // Remove users home dir for apm-dev'ed packages
-               .replace(/\\/g, '/')                             // Switch \ back to / for everyone
-               .replace(/.*(\/(app\.asar|packages\/).*)/, '$1') // Remove everything before app.asar or pacakges
+  normalizePath (pathToNormalize) {
+    return pathToNormalize
+      .replace('file:///', '')  // Sometimes it's a uri
+      .replace(/\\/g, '/')      // Unify path separators across Win/macOS/Linux
+  }
+
+  scrubPath (pathToScrub) {
+    const absolutePath = this.normalizePath(pathToScrub)
+
+    if (this.isBundledFile(absolutePath)) {
+      return this.normalizePath(path.relative(this.resourcePath, absolutePath))
+    } else {
+      return absolutePath
+        .replace(this.normalizePath(fs.getHomeDirectory()), '~') // Remove users home dir
+        .replace(/.*(\/packages\/.*)/, '$1') // Remove everything before app.asar or packages
+    }
   }
 
   getDefaultNotificationParams () {
@@ -107,7 +118,7 @@ export default class Reporter {
 
     const topFrame = this.parseStackTrace(error)[0]
     const fileName = topFrame ? topFrame.getFileName() : null
-    return fileName && (isBundledFile(fileName) || isTeletypeFile(fileName))
+    return fileName && (this.isBundledFile(fileName) || this.isTeletypeFile(fileName))
   }
 
   parseStackTrace (error) {
@@ -240,16 +251,17 @@ export default class Reporter {
   setRequestFunction (requestFunction) {
     this.request = requestFunction
   }
-}
 
-function isBundledFile (fileName) {
-  return fileName.indexOf(atom.getLoadSettings().resourcePath) === 0
-}
+  isBundledFile (fileName) {
+    return this.normalizePath(fileName).indexOf(this.resourcePath) === 0
+  }
 
-function isTeletypeFile (fileName) {
-  const teletypePath = atom.packages.resolvePackagePath('teletype')
-  return teletypePath && fileName.indexOf(teletypePath) === 0
+  isTeletypeFile (fileName) {
+    const teletypePath = atom.packages.resolvePackagePath('teletype')
+    return teletypePath && this.normalizePath(fileName).indexOf(teletypePath) === 0
+  }
 }
 
+
 Reporter.API_KEY = API_KEY
 Reporter.LIB_VERSION = LIB_VERSION

spec/reporter-spec.js

@@ -1,6 +1,8 @@
 const Reporter = require('../lib/reporter')
 const semver = require('semver')
 const os = require('os')
+const path = require('path')
+const fs = require('fs-plus')
 let osVersion = `${os.platform()}-${os.arch()}-${os.release()}`
 
 let getReleaseChannel = version => {
@@ -12,7 +14,7 @@ let getReleaseChannel = version => {
 }
 
 describe("Reporter", () => {
-  let reporter, requests, initialStackTraceLimit, mockActivePackages
+  let reporter, requests, initialStackTraceLimit, initialFsGetHomeDirectory, mockActivePackages
 
   beforeEach(() => {
     reporter = new Reporter({
@@ -27,12 +29,24 @@ describe("Reporter", () => {
     initialStackTraceLimit = Error.stackTraceLimit
     Error.stackTraceLimit = 1
 
+    initialFsGetHomeDirectory = fs.getHomeDirectory
   })
 
-  afterEach(() => Error.stackTraceLimit = initialStackTraceLimit)
+  afterEach(() => {
+    fs.getHomeDirectory = initialFsGetHomeDirectory
+    Error.stackTraceLimit = initialStackTraceLimit
+  })
 
   describe(".reportUncaughtException(error)", () => {
-    it("posts errors to bugsnag", () => {
+    it("posts errors originated inside Atom Core to BugSnag", () => {
+      const repositoryRootPath = path.join(__dirname, '..')
+      reporter = new Reporter({
+        request: (url, options) => requests.push(Object.assign({url}, options)),
+        alwaysReport: true,
+        reportPreviousErrors: false,
+        resourcePath: repositoryRootPath
+      })
+
       let error = new Error()
       Error.captureStackTrace(error)
       reporter.reportUncaughtException(error)
@@ -45,11 +59,58 @@ describe("Reporter", () => {
       expect(request.headers.get("Content-Type")).toBe("application/json")
       let body = JSON.parse(request.body)
 
-      // asserting the correct path is difficult on CI. let's do 'close enough'.
-      expect(body.events[0].exceptions[0].stacktrace[0].file).toMatch(/reporter-spec/)
-      expect(body.events[0].exceptions[0].stacktrace[0].file).not.toMatch(/\\/)
-      delete body.events[0].exceptions[0].stacktrace[0].file
-      delete body.events[0].exceptions[0].stacktrace[0].inProject
+      expect(body).toEqual({
+        "apiKey": Reporter.API_KEY,
+        "notifier": {
+          "name": "Atom",
+          "version": Reporter.LIB_VERSION,
+          "url": "https://www.atom.io"
+        },
+        "events": [
+          {
+            "payloadVersion": "2",
+            "exceptions": [
+              {
+                "errorClass": "Error",
+                "message": "",
+                "stacktrace": [
+                  {
+                    "method": semver.gt(process.versions.electron, '1.6.0') ? 'Spec.it' : 'it',
+                    "file": "spec/reporter-spec.js",
+                    "lineNumber": lineNumber,
+                    "columnNumber": columnNumber,
+                    "inProject": true
+                  }
+                ]
+              }
+            ],
+            "severity": "error",
+            "user": {},
+            "app": {
+              "version": atom.getVersion(),
+              "releaseStage": getReleaseChannel(atom.getVersion())
+            },
+            "device": {
+              "osVersion": osVersion
+            }
+          }
+        ]
+      });})
+
+    it("posts errors originated outside Atom Core to BugSnag", () => {
+      fs.getHomeDirectory = () => path.join(__dirname, '..', '..')
+
+      let error = new Error()
+      Error.captureStackTrace(error)
+      reporter.reportUncaughtException(error)
+      let [lineNumber, columnNumber] = error.stack.match(/.js:(\d+):(\d+)/).slice(1).map(s => parseInt(s))
+
+      expect(requests.length).toBe(1)
+      let [request] = requests
+      expect(request.method).toBe("POST")
+      expect(request.url).toBe("https://notify.bugsnag.com")
+      expect(request.headers.get("Content-Type")).toBe("application/json")
+      let body = JSON.parse(request.body)
 
       expect(body).toEqual({
         "apiKey": Reporter.API_KEY,
@@ -68,8 +129,10 @@ describe("Reporter", () => {
                 "stacktrace": [
                   {
                     "method": semver.gt(process.versions.electron, '1.6.0') ? 'Spec.it' : 'it',
+                    "file": '~/exception-reporting/spec/reporter-spec.js',
                     "lineNumber": lineNumber,
-                    "columnNumber": columnNumber
+                    "columnNumber": columnNumber,
+                    "inProject": true
                   }
                 ]
               }
@@ -210,6 +273,8 @@ describe("Reporter", () => {
 
   describe(".reportFailedAssertion(error)", () => {
     it("posts warnings to bugsnag", () => {
+      fs.getHomeDirectory = () => path.join(__dirname, '..', '..')
+
       let error = new Error()
       Error.captureStackTrace(error)
       reporter.reportFailedAssertion(error)
@@ -222,12 +287,6 @@ describe("Reporter", () => {
       expect(request.headers.get("Content-Type")).toBe("application/json")
       let body = JSON.parse(request.body)
 
-      // asserting the correct path is difficult on CI. let's do 'close enough'.
-      expect(body.events[0].exceptions[0].stacktrace[0].file).toMatch(/reporter-spec/)
-      expect(body.events[0].exceptions[0].stacktrace[0].file).not.toMatch(/\\/)
-      delete body.events[0].exceptions[0].stacktrace[0].file
-      delete body.events[0].exceptions[0].stacktrace[0].inProject
-
       expect(body).toEqual({
         "apiKey": Reporter.API_KEY,
         "notifier": {
@@ -245,8 +304,10 @@ describe("Reporter", () => {
                 "stacktrace": [
                   {
                     "method": semver.gt(process.versions.electron, '1.6.0') ? 'Spec.it' : 'it',
+                    "file": '~/exception-reporting/spec/reporter-spec.js',
                     "lineNumber": lineNumber,
-                    "columnNumber": columnNumber
+                    "columnNumber": columnNumber,
+                    "inProject": true
                   }
                 ]
               }
@@ -405,7 +466,6 @@ describe("Reporter", () => {
       const lastRequest = requests[requests.length - 1]
       const body = JSON.parse(lastRequest.body)
 
-      console.log(body);
       expect(body.events[0].metaData.previousErrors).toEqual(['A', 'B'])
       expect(body.events[0].metaData.previousAssertionFailures).toEqual(['X', 'Y'])
     })