Enable installation tests #3198

Merged
merged 4 commits into from Mar 21, 2013

2 participants

@dangoor

This pull request makes it possible for the extension installation tests to run by enabling the deletion of directories via Node.

Note: to keep this pull request clean, I have not yet committed the node_modules used by the new TestingDomain. To run the ExtensionInstallation tests here, you'll need to:

cd test/node
npm install

I wanted to get this committed so that a couple of basic extension installation tests can run end-to-end. Ultimately, I'd like to see the TestingDomain be able to run jasmine-node tests with the results reported in the client side test runner.

@dangoor

@njx or @peterflynn want to take a look at this?

@njx njx was assigned Mar 21, 2013
@njx
Adobe Systems Incorporated member

I'll take a look.

@njx njx commented on the diff Mar 21, 2013
test/spec/ExtensionInstallation-test.js
@@ -100,7 +101,7 @@ define(function (require, exports, module) {
var realGetUserExtensionPath, realLoadExtension, lastExtensionLoad;
function mockGetUserExtensionPath() {
- return extensionsRoot + "/extensions/user";
+ return extensionsRoot + "/user";
@njx
Adobe Systems Incorporated member
njx added a line comment Mar 21, 2013

Need to mkdir() this folder if it doesn't exist.

@dangoor
dangoor added a line comment Mar 21, 2013

Yeah, the node side will take care of this but the behavior is bad if something is amiss.. will fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@njx njx and 1 other commented on an outdated diff Mar 21, 2013
test/spec/ExtensionInstallation-test.js
@@ -119,35 +120,73 @@ define(function (require, exports, module) {
lastExtensionLoad = {};
realLoadExtension = ExtensionLoader.loadExtension;
ExtensionLoader.loadExtension = mockLoadExtension;
+
+ var nfs = null;
+
+ runs(function () {
+ NativeFileSystem.requestNativeFileSystem(extensionsRoot, function (fs) {
+ nfs = fs;
+ });
+ });
+ waitsFor(function () { return nfs; }, 1000);
@njx
Adobe Systems Incorporated member
njx added a line comment Mar 21, 2013

Let's add a description here (third arg) in case it does time out.

@dangoor
dangoor added a line comment Mar 21, 2013

OK, good call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@njx njx commented on the diff Mar 21, 2013
test/spec/ExtensionInstallation-test.js
});
- // These tests are disabled, because they work fine the first time
- // but cannot run twice unless the destination directory is deleted.
- xit("extensions should install and load", function () {
+ it("extensions should install and load", function () {
@njx
Adobe Systems Incorporated member
njx added a line comment Mar 21, 2013

Is there a way we could check that the extension actually loaded? Maybe have the test extension set some global flag?

@dangoor
dangoor added a line comment Mar 21, 2013

It may not have been clear, but it was mocked. The extension is not allowed to load, but the call that would have loaded it was intercepted and its parameters confirmed.

@njx
Adobe Systems Incorporated member
njx added a line comment Mar 21, 2013

Ah, sorry, should've understood that. (It's what I get for only reading diffs :))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@njx njx and 1 other commented on an outdated diff Mar 21, 2013
test/spec/ExtensionInstallation-test.js
runs(function () {
expect(packageData.errors.length).toEqual(0);
expect(packageData.name).toEqual("basic-valid-extension");
expect(lastExtensionLoad.name).toEqual("basic-valid-extension");
expect(lastExtensionLoad.config.baseUrl).toEqual(mockGetUserExtensionPath() + "/basic-valid-extension");
expect(lastExtensionLoad.entryPoint).toEqual("main");
+ this.nfs.root.getDirectory(mockGetUserExtensionPath() + "/basic-valid-extension", { create: false },
+ function () {
@njx
Adobe Systems Incorporated member
njx added a line comment Mar 21, 2013

Ignore my comment about not understanding this...I didn't get that the expect() below actually fails :)

@njx
Adobe Systems Incorporated member
njx added a line comment Mar 21, 2013

Should we test that the main.js exists as well?

@dangoor
dangoor added a line comment Mar 21, 2013

Yeah, I still need to get to some way to set a message for a failure. Jasmine doesn't even have a way to simply declare a failure!

Sure, I can check for the presence of main.js. Good idea.

@dangoor
dangoor added a line comment Mar 21, 2013

Actually, I just changed the check to look for the main.js file in that directory, rather than looking for the directory and the file independently, since the directory has to exist for the file to be there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@njx njx and 1 other commented on an outdated diff Mar 21, 2013
test/spec/SpecRunnerUtils.js
+ * @param {!string} path Path to remove
+ * @return {$.Promise} Resolved when the path is removed, rejected if there was a problem
+ */
+ function remove(path) {
+ var d = new $.Deferred();
+ var nodeDeferred = brackets.testing.getNodeConnectionDeferred();
+ nodeDeferred.done(function (connection) {
+ if (connection.connected()) {
+ connection.domains.testing.remove(path)
+ .done(function () {
+ d.resolve();
+ })
+ .fail(function () {
+ d.reject();
+ });
+ }
@njx
Adobe Systems Incorporated member
njx added a line comment Mar 21, 2013

Should have rejections for the if and nodeDeferred failure cases.

@dangoor
dangoor added a line comment Mar 21, 2013

OK, added.

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

Review done. Main issue is the initial creation of the test folder.

dangoor added some commits Mar 21, 2013
@dangoor dangoor addressed review comments
* does not fail if the test/temp/extensions directory does not exist
* reject promise in other failure cases for SpecRunnerUtils.remove
* added description of timeout case for requesting the temp filesystem
* checks for existence of main.js file
f868882
@dangoor dangoor add node_modules used in testing c6373ff
@dangoor

Review comments addressed and I added the node_modules used for the tests. (Note that there is no new third party code in that node_modules. These same modules are also used in the extension installation code.)

@njx
Adobe Systems Incorporated member

Just one last issue--travis is failing because we're running jshint on the new node modules. Not sure why. I'll look at this in an hour if you haven't gotten to it by then :)

@njx
Adobe Systems Incorporated member

Travis fixed. Merging.

@njx njx merged commit 5a0fbcd into master Mar 21, 2013

1 check passed

Details default The Travis build passed
@njx njx deleted the dangoor/testing-domain branch Mar 21, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment