From 388428b0740764fa55ae7909321adafc468b3efa Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Tue, 23 Dec 2014 16:20:37 -0800 Subject: [PATCH 1/4] Fix logic error when exception is thrown in config observer --- spec/config-spec.coffee | 12 +++++++++--- src/config.coffee | 21 ++++++++++----------- 2 files changed, 19 insertions(+), 14 deletions(-) diff --git a/spec/config-spec.coffee b/spec/config-spec.coffee index 9640fae3e..4940678fe 100644 --- a/spec/config-spec.coffee +++ b/spec/config-spec.coffee @@ -493,13 +493,19 @@ describe "Config", -> expect(observeHandler).not.toHaveBeenCalled() it "fires the callback every time the observed value changes", -> - observeHandler.reset() # clear the initial call atom.config.set('foo.bar.baz', "value 2") expect(observeHandler).toHaveBeenCalledWith({newValue: 'value 2', oldValue: 'value 1'}) observeHandler.reset() - atom.config.set('foo.bar.baz', "value 1") + observeHandler.andCallFake -> throw new Error("oops") + expect(-> atom.config.set('foo.bar.baz', "value 1")).toThrow("oops") expect(observeHandler).toHaveBeenCalledWith({newValue: 'value 1', oldValue: 'value 2'}) + observeHandler.reset() + + # Regression: exception in earlier handler shouldn't put observer + # into a bad state. + atom.config.set('something.else', "new value") + expect(observeHandler).not.toHaveBeenCalled() describe 'when a keyPath is not specified', -> beforeEach -> @@ -567,7 +573,7 @@ describe "Config", -> atom.config.set("foo.bar.baz", "i'm back") expect(observeHandler).toHaveBeenCalledWith("i'm back") - it "does not fire the callback once the observe subscription is off'ed", -> + it "does not fire the callback once the subscription is disposed", -> observeHandler.reset() # clear the initial call observeSubscription.dispose() atom.config.set('foo.bar.baz', "value 2") diff --git a/src/config.coffee b/src/config.coffee index d0d8f7d2f..9db755f2f 100644 --- a/src/config.coffee +++ b/src/config.coffee @@ -882,13 +882,12 @@ class Config onDidChangeKeyPath: (keyPath, callback) -> oldValue = @get(keyPath) - - didChange = => + @emitter.on 'did-change', => newValue = @get(keyPath) - callback({oldValue, newValue}) unless _.isEqual(oldValue, newValue) - oldValue = newValue - - @emitter.on 'did-change', didChange + unless _.isEqual(oldValue, newValue) + event = {oldValue, newValue} + oldValue = newValue + callback(event) isSubKeyPath: (keyPath, subKeyPath) -> return false unless keyPath? and subKeyPath? @@ -1003,12 +1002,12 @@ class Config onDidChangeScopedKeyPath: (scope, keyPath, callback) -> oldValue = @get(keyPath, {scope}) - didChange = => + @emitter.on 'did-change', => newValue = @get(keyPath, {scope}) - callback({oldValue, newValue}) unless _.isEqual(oldValue, newValue) - oldValue = newValue - - @emitter.on 'did-change', didChange + unless _.isEqual(oldValue, newValue) + event = {oldValue, newValue} + oldValue = newValue + callback(event) # TODO: figure out how to change / remove this. The return value is awkward. # * language mode uses it for one thing. From d90daf07f8502b6e6f33a8844a2b8e7cd3ef4c92 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Tue, 23 Dec 2014 16:47:09 -0800 Subject: [PATCH 2/4] In PackageManager::activatePackage reject, don't throw --- spec/package-manager-spec.coffee | 76 +++++++++++++++++++++++++------- spec/theme-manager-spec.coffee | 4 +- src/package-manager.coffee | 11 ++--- 3 files changed, 70 insertions(+), 21 deletions(-) diff --git a/spec/package-manager-spec.coffee b/spec/package-manager-spec.coffee index bac6270f8..aa49f1076 100644 --- a/spec/package-manager-spec.coffee +++ b/spec/package-manager-spec.coffee @@ -3,19 +3,36 @@ Package = require '../src/package' describe "PackageManager", -> workspaceElement = null + beforeEach -> workspaceElement = atom.views.getView(atom.workspace) describe "::loadPackage(name)", -> - it "continues if the package has an invalid package.json", -> - spyOn(console, 'warn') + beforeEach -> atom.config.set("core.disabledPackages", []) - expect(-> atom.packages.loadPackage("package-with-broken-package-json")).not.toThrow() - it "continues if the package has an invalid keymap", -> + it "returns the package", -> + pack = atom.packages.loadPackage("package-with-index") + expect(pack instanceof Package).toBe true + expect(pack.metadata.name).toBe "package-with-index" + + it "returns the package if it has an invalid keymap", -> spyOn(console, 'warn') - atom.config.set("core.disabledPackages", []) - expect(-> atom.packages.loadPackage("package-with-broken-keymap")).not.toThrow() + pack = atom.packages.loadPackage("package-with-broken-keymap") + expect(pack instanceof Package).toBe true + expect(pack.metadata.name).toBe "package-with-broken-keymap" + + it "returns null if the package has an invalid package.json", -> + spyOn(console, 'warn') + expect(atom.packages.loadPackage("package-with-broken-package-json")).toBeNull() + expect(console.warn.callCount).toBe(1) + expect(console.warn.argsForCall[0][0]).toContain("Failed to load package.json") + + it "returns null if the package is not found in any package directory", -> + spyOn(console, 'warn') + expect(atom.packages.loadPackage("this-package-cannot-be-found")).toBeNull() + expect(console.warn.callCount).toBe(1) + expect(console.warn.argsForCall[0][0]).toContain("Could not resolve") describe "::unloadPackage(name)", -> describe "when the package is active", -> @@ -197,11 +214,35 @@ describe "PackageManager", -> runs -> expect(pack.mainModule.activate).toHaveBeenCalledWith({someNumber: 77}) - it "logs warning instead of throwing an exception if the package fails to load", -> - atom.config.set("core.disabledPackages", []) - spyOn(console, "warn") - expect(-> atom.packages.activatePackage("package-that-throws-an-exception")).not.toThrow() - expect(console.warn).toHaveBeenCalled() + describe "when the package throws an error while loading", -> + it "logs warning instead of throwing an exception", -> + atom.config.set("core.disabledPackages", []) + spyOn(console, "warn") + expect(-> atom.packages.activatePackage("package-that-throws-an-exception")).not.toThrow() + expect(console.warn).toHaveBeenCalled() + + describe "when the package is not found", -> + it "rejects the promise", -> + atom.config.set("core.disabledPackages", []) + + onSuccess = jasmine.createSpy('onSuccess') + onFailure = jasmine.createSpy('onFailure') + spyOn(console, 'warn') + + atom.packages.activatePackage("this-doesnt-exist").then( + onSuccess, + onFailure + ) + + waitsFor "promise to be rejected", 1000, -> + onFailure.callCount > 0 + + runs -> + expect(console.warn.callCount).toBe 1 + expect(onFailure.mostRecentCall.args[0] instanceof Error).toBe true + expect(onFailure.mostRecentCall.args[0].message).toContain( + "Failed to load package 'this-doesnt-exist'" + ) describe "keymap loading", -> describe "when the metadata does not contain a 'keymaps' manifest", -> @@ -552,9 +593,9 @@ describe "PackageManager", -> themes = themeActivator.mostRecentCall.args[0] expect(['theme']).toContain(theme.getType()) for theme in themes - describe "::enablePackage() and ::disablePackage()", -> + describe "::enablePackage(id) and ::disablePackage(id)", -> describe "with packages", -> - it ".enablePackage() enables a disabled package", -> + it "enables a disabled package", -> packageName = 'package-with-main' atom.config.pushAtKeyPath('core.disabledPackages', packageName) atom.packages.observeDisabledPackages() @@ -572,7 +613,7 @@ describe "PackageManager", -> expect(activatedPackages).toContain(pack) expect(atom.config.get('core.disabledPackages')).not.toContain packageName - it ".disablePackage() disables an enabled package", -> + it "disables an enabled package", -> packageName = 'package-with-main' waitsForPromise -> atom.packages.activatePackage(packageName) @@ -587,6 +628,11 @@ describe "PackageManager", -> expect(activatedPackages).not.toContain(pack) expect(atom.config.get('core.disabledPackages')).toContain packageName + it "returns null if the package cannot be loaded", -> + spyOn(console, 'warn') + expect(atom.packages.enablePackage("this-doesnt-exist")).toBeNull() + expect(console.warn.callCount).toBe 1 + describe "with themes", -> reloadedHandler = null @@ -597,7 +643,7 @@ describe "PackageManager", -> afterEach -> atom.themes.deactivateThemes() - it ".enablePackage() and .disablePackage() enables and disables a theme", -> + it "enables and disables a theme", -> packageName = 'theme-with-package-file' expect(atom.config.get('core.themes')).not.toContain packageName diff --git a/spec/theme-manager-spec.coffee b/spec/theme-manager-spec.coffee index a5660ff61..dffec95b7 100644 --- a/spec/theme-manager-spec.coffee +++ b/spec/theme-manager-spec.coffee @@ -131,7 +131,9 @@ describe "ThemeManager", -> describe "when a theme fails to load", -> it "logs a warning", -> spyOn(console, 'warn') - expect(-> atom.packages.activatePackage('a-theme-that-will-not-be-found')).toThrow() + atom.packages.activatePackage('a-theme-that-will-not-be-found') + expect(console.warn.callCount).toBe 1 + expect(console.warn.argsForCall[0][0]).toContain "Could not resolve 'a-theme-that-will-not-be-found'" describe "::requireStylesheet(path)", -> beforeEach -> diff --git a/src/package-manager.coffee b/src/package-manager.coffee index 31b9046a2..6edad3007 100644 --- a/src/package-manager.coffee +++ b/src/package-manager.coffee @@ -295,12 +295,12 @@ class PackageManager pack = new Package(packagePath, metadata) pack.load() @loadedPackages[pack.name] = pack - pack + return pack catch error console.warn "Failed to load package.json '#{path.basename(packagePath)}'", error.stack ? error - else - throw new Error("Could not resolve '#{nameOrPath}' to a package path") + console.warn "Could not resolve '#{nameOrPath}' to a package path" + null unloadPackages: -> @unloadPackage(name) for name in _.keys(@loadedPackages) @@ -337,11 +337,12 @@ class PackageManager activatePackage: (name) -> if pack = @getActivePackage(name) Q(pack) - else - pack = @loadPackage(name) + else if pack = @loadPackage(name) pack.activate().then => @activePackages[pack.name] = pack pack + else + Q.reject(new Error("Failed to load package '#{name}'")) # Deactivate all packages deactivatePackages: -> From b1a3d89af319357c9b608d93d24c512053124e20 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Tue, 23 Dec 2014 17:05:21 -0800 Subject: [PATCH 3/4] Fix grammar in spec description --- spec/package-manager-spec.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/package-manager-spec.coffee b/spec/package-manager-spec.coffee index aa49f1076..9550e31a6 100644 --- a/spec/package-manager-spec.coffee +++ b/spec/package-manager-spec.coffee @@ -215,7 +215,7 @@ describe "PackageManager", -> expect(pack.mainModule.activate).toHaveBeenCalledWith({someNumber: 77}) describe "when the package throws an error while loading", -> - it "logs warning instead of throwing an exception", -> + it "logs a warning instead of throwing an exception", -> atom.config.set("core.disabledPackages", []) spyOn(console, "warn") expect(-> atom.packages.activatePackage("package-that-throws-an-exception")).not.toThrow() From c3280924632ee58ed07320f97ff8b13b52dfd04b Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Tue, 23 Dec 2014 17:14:25 -0800 Subject: [PATCH 4/4] :lipstick: package-manager-spec --- spec/package-manager-spec.coffee | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/spec/package-manager-spec.coffee b/spec/package-manager-spec.coffee index 9550e31a6..2eee6c0d0 100644 --- a/spec/package-manager-spec.coffee +++ b/spec/package-manager-spec.coffee @@ -229,20 +229,15 @@ describe "PackageManager", -> onFailure = jasmine.createSpy('onFailure') spyOn(console, 'warn') - atom.packages.activatePackage("this-doesnt-exist").then( - onSuccess, - onFailure - ) + atom.packages.activatePackage("this-doesnt-exist").then(onSuccess, onFailure) - waitsFor "promise to be rejected", 1000, -> + waitsFor "promise to be rejected", -> onFailure.callCount > 0 runs -> expect(console.warn.callCount).toBe 1 expect(onFailure.mostRecentCall.args[0] instanceof Error).toBe true - expect(onFailure.mostRecentCall.args[0].message).toContain( - "Failed to load package 'this-doesnt-exist'" - ) + expect(onFailure.mostRecentCall.args[0].message).toContain "Failed to load package 'this-doesnt-exist'" describe "keymap loading", -> describe "when the metadata does not contain a 'keymaps' manifest", ->