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

Fix async issues in brackets-shell #1185

Merged
merged 9 commits into from
Jul 12, 2012
Merged

Conversation

jasonsanjose
Copy link
Member

  • Fix unit tests with brackets.forceAsync = true.
  • Revert some PerfUtils.markStart() calls to unique strings.
  • Return $.Promise in cases where file access is async.
  • Cleanup document identifiers
  • Allow multiple calls to getDocumentForPath

Tested on 10.7 using brackets-app with forceAsync=true and on windows with the sprint 10 brackets-shell build. I did hit 1 lowlevelfileIO failure on windows 7 that isn't related to the scope of the sprint 11 CEF3 bugs.

….markStart() calls to unique strings. Return $.Promise in cases where file access is async.
@ghost ghost assigned peterflynn Jun 28, 2012

var result = new $.Deferred(), promise = result.promise();
var result = new $.Deferred();

if (!fullPath) {
console.log("doOpen() called without fullPath");
result.reject();
return promise;
Copy link
Member

Choose a reason for hiding this comment

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

There's still a reference to promise here. I'm fine with removing the var though.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@peterflynn
Copy link
Member

Done reviewing

@jasonsanjose
Copy link
Member Author

Addressed comments in the last 2 commits. However, I'm seeing failures on windows in CEF1 that I need to review.

@jasonsanjose
Copy link
Member Author

@peterflynn false alarm. My preferences were wedged. Unit tests pass for me on mac and win using CEF1 with and without forceAsync.

@peterflynn
Copy link
Member

@jason-sanjose: did you check in the actual brackets-shell too? (The Win build is still on zerowing somewhere... not sure about a Mac build, but I assume Raymond could get you one today).

@jasonsanjose
Copy link
Member Author

@peterflynn Just got it working. Had to run each suite individually in brackets-shell as well. I hit 3 more failures:

  • inlineeditorproviders should close inline when file is closed without saving changes
  • lowlevelfileio should return an error if the file doesn't exist
  • nativefilesystem should read a directory from disk

I'm taking a look at the first one now. I didn't see the other 2 accounted for in brackets-shell. I'll update in a bit.

@jasonsanjose
Copy link
Member Author

I filed https://github.com/adobe/brackets-shell/issues/26 to log the LowLevelFileIO failure. I also fixed the inline editor provider failure (another async issue). The nativefilesystem test is failing because it can't find the matcher that was added via addMatcher(). Not sure how that happened. I think this pull request is in the clear though.

@jasonsanjose
Copy link
Member Author

@peterflynn I got brackets-shell on win to pass fully. I had to fix some async issues in NativeFileSystem-tests. No fixes to core.

SpecRunnerUtils.clickDialogButton(Dialogs.DIALOG_BTN_DONTSAVE);

waitsForDone(promise);
Copy link
Member

Choose a reason for hiding this comment

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

clickDialogButton() is already supposed to wait for the dialog to close. If this line is needed, something's broken there (I guess its no longer guaranteed that timeouts in one window are sequenced interleaved with timeouts in another window?). We should try to fix clickDialogButton() itself instead of patching around it here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, thanks for catching that. It was unecessary.

@peterflynn
Copy link
Member

I think with the clickDialogButton() issue sorted out we should be good to merge. The higher-level issues maybe we could talk about in next week's architecture meeting?

@jasonsanjose
Copy link
Member Author

Fixed clickDialogButton() issue. Merged with master. Ready to go. Thanks @peterflynn!

@peterflynn
Copy link
Member

Cool -- merging.

peterflynn added a commit that referenced this pull request Jul 12, 2012
Fix async issues in brackets-shell that were causing unit tests to fail. Change more unit tests to use waitsForDone().
@peterflynn peterflynn merged commit b559827 into master Jul 12, 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.

3 participants