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

Fix unit tests broken by modal bar animation #4800

Merged
merged 1 commit into from
Aug 16, 2013
Merged

Conversation

njx
Copy link
Contributor

@njx njx commented Aug 16, 2013

The modal bar animation added by @larz0 added a bit of asynchronicity to the modal bar: it now isn't removed from the DOM until the close animation completes. This pull request fixes up the unit tests so that they wait for the previous modal bar to close (and forcibly remove it at the end of the test) before continuing.

Also, some unit tests were breaking because the modal bar was never removing itself from the DOM. This was due to a subtle issue: the new modal bar code was using the .hide class, but that class is also defined by Bootstrap to set display: none. That wasn't affecting the modal bar's behavior in Brackets itself, but was affecting it in the test runner, possibly because stylesheets are loaded in a different order. So I just renamed the class to .modal-bar-hide to avoid getting contaminated by the Bootstrap rule.

@@ -214,74 +226,89 @@ define(function (require, exports, module) {
});

it("should Find Next after search bar closed, including wraparound", function () {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of these changes were just to wrap runs() blocks around code that originally did expectSearchBarClosed().

@njx
Copy link
Contributor Author

njx commented Aug 16, 2013

I highly recommend reviewing with ?w=1 :)

@njx
Copy link
Contributor Author

njx commented Aug 16, 2013

@gruehle or @peterflynn--interested in taking this one? Want to get it checked in today since the unit tests are broken.

Note that this does mean that the timing of when modal bars appear/disappear has changed...but I'm inclined not to worry about it too much unless we find actual user-visible issues.

@njx
Copy link
Contributor Author

njx commented Aug 16, 2013

Oops, retagging @peterflynn

@njx
Copy link
Contributor Author

njx commented Aug 16, 2013

For #4797

@gruehle
Copy link
Member

gruehle commented Aug 16, 2013

Looks good. Merging.

gruehle added a commit that referenced this pull request Aug 16, 2013
Fix unit tests broken by modal bar animation
@gruehle gruehle merged commit c4601cc into master Aug 16, 2013
@gruehle gruehle deleted the nj/fix-find-tests branch August 16, 2013 23:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants