Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

fix module loading for SpecRunnerUtils #1250

Merged
merged 3 commits into from
Jul 16, 2012
Merged

Conversation

jasonsanjose
Copy link
Member

The first testWindow didn't close because multiple instances of the SpecRunnerUtils module were loaded. Fixes adobe/brackets-shell#33.

@jasonsanjose
Copy link
Member Author

Hmm, I thought this was working but Raymond reported it failed. Investigating.

@peterflynn
Copy link
Member

@jason-sanjose: it still repros for me too, fwiw

@jasonsanjose
Copy link
Member Author

Forced jasmine init to happen after extension loading. The bug where the window remains open was due to the beforeEach() callback was added to the wrong spec. In this case, jasmine's currentSpec was updated unexpectedly to the last extension unit test loaded. Ready for re-review @peterflynn.

@ghost ghost assigned peterflynn Jul 16, 2012
@peterflynn
Copy link
Member

Question: do we still need all the "./SpecRunnerUtils.js" -> "spec/SpecRunnerUtils" changes? Also, remind me what that means in Require again? :-)

@jasonsanjose
Copy link
Member Author

For consistency, yes, we need that require change. Without this, we basically have 2 versions of SpecRunnerUtils modules loaded. RequireJS treats entries with .js as a regular document relative URL instead of using the baseURL + paths.

@@ -153,10 +158,9 @@ define(function (require, exports, module) {
paths: config.paths
};
return processExtension(item, extConfig, entryPoint);
}).done(function () {
}).always(function () {
// Always resolve the promise even when the extension entry point is missing
result.resolve();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bear in mind that the fail() case for extension loading is never invoked: if an extension doesn't exist or crashes during init, we just never get a response instead. So even though this is now attached to always(), the result will still never resolve if there's any sort of error. (I don't think this has any effect on the rest of your patch, but just thought I should point it out for clarity).

For details see here: #954 (comment)

@peterflynn
Copy link
Member

The SpecRunner.js init() stuff looks much cleaner right now -- nice!

@peterflynn
Copy link
Member

Ok, looks good! I can confirm it fixes adobe/brackets-shell#33 for me too. Merging now...

peterflynn added a commit that referenced this pull request Jul 16, 2012
fix module loading for SpecRunnerUtils - ensures unit tests that open a new window actually runs instead of silently getting ignored (in brackets-shell)
@peterflynn peterflynn merged commit 7d2a828 into master Jul 16, 2012
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants