Skip to content

Update unit tests for extension manager keepFile flag #8511

Merged
merged 1 commit into from Aug 7, 2014

3 participants

@jasonsanjose
Adobe Systems Incorporated member

See #8166

@jasonsanjose jasonsanjose commented on the diff Jul 22, 2014
test/spec/ExtensionManager-test.js
@@ -689,8 +689,8 @@ define(function (require, exports, module) {
waitsForDone(ExtensionManager.updateExtensions());
});
runs(function () {
- expect(file.unlink).not.toHaveBeenCalled();
- expect(Package.installUpdate).toHaveBeenCalledWith(filename, id, undefined);
+ expect(file.unlink).toHaveBeenCalled();
@jasonsanjose
Adobe Systems Incorporated member
jasonsanjose added a note Jul 22, 2014

See this change https://github.com/adobe/brackets/pull/8166/files#diff-8c7353c628552869915aed6a9a600033L425. Package.installUpdate always deleted the file prior to the introduction of keepFile.

However, this test was only passing because of an assumption on line 687 that using the andReturn spy method would call through the actual implementation, however, this is not the case. The test would complete without ever following the branch that would lead to unlink.

The test now expects unlink to be called during ExtensionManager.updateExtesnions().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jasonsanjose jasonsanjose commented on the diff Jul 22, 2014
src/extensibility/Package.js
* @return {$.Promise} A promise that is resolved when the extension is successfully
* installed or rejected if there is a problem.
*/
- function installUpdate(path, nameHint, keepFile) {
+ function installUpdate(path, nameHint) {
@jasonsanjose
Adobe Systems Incorporated member
jasonsanjose added a note Jul 22, 2014

Changes responsibility of deleting updates to callers of installUpdate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jasonsanjose
Adobe Systems Incorporated member

@dangoor want to take this one too?

@dangoor
dangoor commented Jul 22, 2014

@jasonsanjose Be happy to... but not today. I'm a bit overloaded.

@dangoor
dangoor commented Jul 22, 2014

oh wait, there's actual code changes in here and not just tests?

@jasonsanjose
Adobe Systems Incorporated member

There were some minor code changes, yes. Mostly for cleanup and to make unit testing easier.

@jasonsanjose jasonsanjose commented on the diff Jul 22, 2014
src/styles/brackets.less
@@ -1661,6 +1661,10 @@ label input {
position: relative;
}
+#install-drop-zone .install-from-url {
+ cursor: pointer;
@jasonsanjose
Adobe Systems Incorporated member
jasonsanjose added a note Jul 22, 2014

Use pointer (hand) for "Install from URL" link.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dangoor
dangoor commented Jul 22, 2014

Ahh. Anything critical for 0.42, or is okay if this PR falls into 0.43?

@jasonsanjose
Adobe Systems Incorporated member

Nah, not critical.

@MarcelGerber MarcelGerber commented on the diff Jul 23, 2014
src/extensibility/ExtensionManager.js
@@ -443,6 +447,10 @@ define(function (require, exports, module) {
* @param {Object} installationResult info about the install provided by the Package.download function
*/
function updateFromDownload(installationResult) {
+ if (installationResult.keepFile === undefined) {
+ installationResult.keepFile = false;
+ }
@MarcelGerber
Adobe Systems Incorporated member
MarcelGerber added a note Jul 23, 2014
installationResult.keepFile = installationResult.keepFile || false;

is probably easier, and will also avoid null

@dangoor
dangoor added a note Aug 7, 2014

This seems fine to me. No one is going to set that value to null.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dangoor dangoor self-assigned this Aug 6, 2014
@dangoor
dangoor commented Aug 7, 2014

Looks good. Merging.

@dangoor dangoor merged commit 9bd6c71 into master Aug 7, 2014

1 check passed

Details continuous-integration/travis-ci The Travis CI build passed
@dangoor dangoor deleted the jasonsanjose/dragdrop-test-update branch Aug 7, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.