Merge pull request #25 from atom/wl-error-notification-detail

Show error output if `apm install` fails
atom · Nov 11, 2017 · 09792df · 09792df
2 parents d1091ee + 4d3ff00
commit 09792df
Show file tree

Hide file tree

Showing 2 changed files with 15 additions and 9 deletions.
diff --git a/lib/update-package-dependencies.js b/lib/update-package-dependencies.js
@@ -22,22 +22,25 @@ module.exports = {
     if (this.process) return // Do not allow multiple apm processes to run
     if (this.updatePackageDependenciesStatusView) this.updatePackageDependenciesStatusView.attach()
 
+    let errorOutput = ''
+
     const command = atom.packages.getApmPath()
-    const args = ['install']
+    const args = ['install', '--no-color']
+    const stderr = output => { errorOutput += output }
     const options = {cwd: this.getActiveProjectPath(), env: Object.assign({}, process.env, {NODE_ENV: 'development'})}
 
     const exit = code => {
       this.process = null
       if (this.updatePackageDependenciesStatusView) this.updatePackageDependenciesStatusView.detach()
 
       if (code === 0) {
-        atom.notifications.addSuccess('Success!', {detail: 'Package dependencies updated.'})
+        atom.notifications.addSuccess('Package dependencies updated')
       } else {
-        atom.notifications.addError('Error!', {detail: 'Failed to update package dependencies.'})
+        atom.notifications.addError('Failed to update package dependencies', {detail: errorOutput, dismissable: true})
       }
     }
 
-    this.process = this.runBufferedProcess({command, args, exit, options})
+    this.process = this.runBufferedProcess({command, args, stderr, exit, options})
   },
 
   // This function exists so that it can be spied on by tests

diff --git a/spec/update-package-dependencies-spec.js b/spec/update-package-dependencies-spec.js
@@ -13,10 +13,10 @@ describe('Update Package Dependencies', () => {
   })
 
   describe('updating package dependencies', () => {
-    let {command, args, exit, options} = {}
+    let {command, args, stderr, exit, options} = {}
     beforeEach(() => {
       spyOn(updatePackageDependencies, 'runBufferedProcess').andCallFake((params) => {
-        ({command, args, exit, options} = params)
+        ({command, args, stderr, exit, options} = params)
         return true // so that this.process isn't null
       })
     })
@@ -34,7 +34,7 @@ describe('Update Package Dependencies', () => {
       } else {
         expect(command.endsWith('\\apm.cmd')).toBe(true)
       }
-      expect(args).toEqual(['install'])
+      expect(args).toEqual(['install', '--no-color'])
       expect(options.cwd).toEqual(projectPath)
     })
 
@@ -96,20 +96,23 @@ describe('Update Package Dependencies', () => {
       it('shows a success notification message', () => {
         const notification = atom.notifications.getNotifications()[0]
         expect(notification.getType()).toEqual('success')
-        expect(notification.getMessage()).toEqual('Success!')
+        expect(notification.getMessage()).toEqual('Package dependencies updated')
       })
     })
 
     describe('when the update fails', () => {
       beforeEach(() => {
         updatePackageDependencies.update()
+        stderr('oh bother')
         exit(127)
       })
 
       it('shows a failure notification', () => {
         const notification = atom.notifications.getNotifications()[0]
         expect(notification.getType()).toEqual('error')
-        expect(notification.getMessage()).toEqual('Error!')
+        expect(notification.getMessage()).toEqual('Failed to update package dependencies')
+        expect(notification.getDetail()).toEqual('oh bother')
+        expect(notification.isDismissable()).toBe(true)
       })
     })
   })