Reuse test windows during integration suites #4618

jasonsanjose · 2013-07-31T22:26:26Z

Updates CodeHint, FileIndexManager and LiveDevelopment integration suites to reuse the same test window.
Adds private CodeHintManager._removeHintProvider method to restore CodeHintManager state for unit tests

jasonsanjose · 2013-07-31T22:26:57Z

jasonsanjose · 2013-07-31T22:34:30Z

test/spec/FileIndexManager-test.js

-                        .done(function (result) {
-                            allFiles = result;
-                        });
+            describe("multiple requests", function () {


The changes in this file look bad, but I simply isolated the following 2 specs since they can't share a test window with others:

should handle differing simultaneous requests without doing extra work

should handle identical simultaneous requests without doing extra work

You can add ?w=1 to the github URL for the file diff to get it to ignore whitespace. (Unfortunately you can't comment in that view.)

Thanks, that make it much easier to review it.

It would be great to have a diff that showed different the code that was moved around. Makes the diff huge and nothing really changed.

TomMalbran · 2013-08-01T00:09:45Z

@jasonsanjose Done with an initial review. I am seeing a failure in should push in memory css changes made before the session starts timeout: timed out after 1000 msec waiting for success SpecRunnerUtils.openProjectFiles. Are you getting that too?

jasonsanjose · 2013-08-01T18:32:57Z

I'm actually seeing a different failure in that spec should push in memory css changes made before the session starts that I could only reproduce twice

TypeError: Cannot read property 'styleSheetId' of undefined
    at Object.reloadCSSForDocument (file://localhost/Applications/Brackets%20Sprint%2028.app/Contents/dev/src/LiveDevelopment/Agents/CSSAgent.js:96:46)
    at CSSDocument.onChange (file://localhost/Applications/Brackets%20Sprint%2028.app/Contents/dev/src/LiveDevelopment/Documents/CSSDocument.js:172:18)
    at Document.x.event.dispatch (file://localhost/Applications/Brackets%20Sprint%2028.app/Contents/dev/src/thirdparty/jquery-2.0.1.min.js:5:9843)
    at Document.x.event.add.y.handle (file://localhost/Applications/Brackets%20Sprint%2028.app/Contents/dev/src/thirdparty/jquery-2.0.1.min.js:5:6626)
    at Object.x.event.trigger (file://localhost/Applications/Brackets%20Sprint%2028.app/Contents/dev/src/thirdparty/jquery-2.0.1.min.js:5:8968)
    at x.fn.extend.triggerHandler (file://localhost/Applications/Brackets%20Sprint%2028.app/Contents/dev/src/thirdparty/jquery-2.0.1.min.js:5:14754)
    at Document._handleEditorChange (file://localhost/Applications/Brackets%20Sprint%2028.app/Contents/dev/src/document/Document.js:372:17)

TomMalbran · 2013-08-01T19:41:00Z

I haven't seen that error, but I still get the one I mentioned. Occasionally it doesn't appear. The weird part is that the file stills open and the live development is done.

One thing I noticed is that testWindow.closeAllDocuments closes the documents but doesn't discards the changes done. So when reopening them, you get the with the old changes. I feel it will be saved to use testWindow.closeAllFiles (implemented in #4598) which closes the open files and discards the changes.

TomMalbran · 2013-08-01T20:47:53Z

I was somehow able to get the same failure that you mention and also saw it at should reapply in-memory css changes after saving changes in html document.

Increasing the files to open timeout to 1000 seems to have fixed the other failure I mentioned.

jasonsanjose · 2013-08-01T20:59:34Z

I'm trying to fix the failure in should close inline editor when file deleted on disk (see #4598) you saw in the inline editor pull request. I'll let you know when I'm done.

jasonsanjose · 2013-08-01T22:16:21Z

Hmm, I tried calling in DocumentManager.closeAll() in a normal brackets session from dev tools. When I dirtied a file then reopened it, the file came back clean with no edits. I followed the call stack and closeAll eventually does the right thing and destroys the editor...I didn't see anything obvious in the test that would cause what you saw.

I prefer closeAllFiles though since it seems cleaner to call the actual command. I'll make the change.

jasonsanjose · 2013-08-01T22:20:37Z

@TomMalbran ready for review. I fixed the inline editor test failure and after increasing the timeout I haven't reproduce the live dev failures.

TomMalbran · 2013-08-01T22:24:50Z

When running the live development test, without closing the window at the end, check the files. Open "simple1.css" into the working set, and you should see it dirty. Maybe the Document/Editor was never destroyed.

I actually tried using closeAllDocuments on the EditorOptionsTests and all tests started to fail since the initial text wasn't the same as what expected as it was the test after the edition from the previous test.

I'll check the new changes.

jasonsanjose · 2013-08-01T22:35:08Z

Ok, I'll try that out. I had to back out one change to DocumentCommandHandlers-test because the spies on Dialog prevented closeAllFiles from working properly.

TomMalbran · 2013-08-01T23:00:42Z

Sounds good. If the test don't care about the content of the file, like the DocumentCommandHandler tests, DocumentManager.closeAll is good. We could also update FILE_CLOSE_ALL to receive a dontShowUI option so that it doesn't show it. I still don't get why I can see the older changes when reopening the files.

The InlineEditors tests work fine. Wasn't able to reproduce the failure. But I did still saw the timeout in the Live Development tests since the default timeout is 1000 and the new timeout is still 1000. I haven't seen any errors with 5000 or 10000 as a timeout on files.

By the way, at which amount of commits we ask for a rebase?

…nager and live dev fix jshint error code review comments optimize project rewrite for inlineeditor tests fix failing inline editor test by forcing FileIndexManager to sync replace closeAllDocuments with closeAllFiles increase timeout for openProjectFiles Use DocumentManager.closeAll() to prevent accidental usage of spies set in Dialog.showSaveDialog

jasonsanjose · 2013-08-01T23:07:58Z

Just rebased! :)

I'm happy with these changes even with that one live development failure. If you're happy and want to merge I can see if this improves run time on our build machines.

TomMalbran · 2013-08-01T23:14:06Z

Sure, maybe the test run a bit slower on my computer with all the stuff I have open anyway. And the test doesn't always fail.

Merging.

…dows Reuse test windows during integration suites

TomMalbran · 2013-08-01T23:14:59Z

Let me know if it improved or if anything failed :)

jasonsanjose · 2013-08-01T23:29:13Z

Much improved so far. Finished under 6 minutes on windows and under 4 minutes on mac. I'll need a few more test runs to feel confident. For the last month or so they would hit the 15min timeout. Our current theory is that we've got a memory leak (#4185) that is magnified in the test runs due to how many new windows we open.

TomMalbran · 2013-08-01T23:43:53Z

Awesome :)

I was checking Documentmanager.closeAll and it seems to work fine. Maybe what I see is a result of the failure test.

jasonsanjose reviewed Jul 31, 2013
View reviewed changes

ghost assigned TomMalbran Aug 1, 2013

TomMalbran added a commit that referenced this pull request Aug 1, 2013

Merge pull request #4618 from adobe/jasonsanjose/integration-test-win…

3855dc6

…dows Reuse test windows during integration suites

TomMalbran merged commit 3855dc6 into master Aug 1, 2013

TomMalbran deleted the jasonsanjose/integration-test-windows branch August 1, 2013 23:14

jasonsanjose mentioned this pull request Aug 1, 2013

Integration tests do not complete on build machines (intermittent) #4547

Closed

core-ai-bot mentioned this pull request Aug 29, 2021

[CLOSED] Reuse test windows during integration suites brackets-archive/bracketsIssues#4270

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reuse test windows during integration suites #4618

Reuse test windows during integration suites #4618

jasonsanjose commented Jul 31, 2013

jasonsanjose commented Jul 31, 2013

jasonsanjose Jul 31, 2013

njx Jul 31, 2013

TomMalbran Aug 1, 2013

TomMalbran commented Aug 1, 2013

jasonsanjose commented Aug 1, 2013

TomMalbran commented Aug 1, 2013

TomMalbran commented Aug 1, 2013

jasonsanjose commented Aug 1, 2013

jasonsanjose commented Aug 1, 2013

jasonsanjose commented Aug 1, 2013

TomMalbran commented Aug 1, 2013

jasonsanjose commented Aug 1, 2013

TomMalbran commented Aug 1, 2013

jasonsanjose commented Aug 1, 2013

TomMalbran commented Aug 1, 2013

TomMalbran commented Aug 1, 2013

jasonsanjose commented Aug 1, 2013

TomMalbran commented Aug 1, 2013

Reuse test windows during integration suites #4618

Reuse test windows during integration suites #4618

Conversation

jasonsanjose commented Jul 31, 2013

jasonsanjose commented Jul 31, 2013

jasonsanjose Jul 31, 2013

Choose a reason for hiding this comment

njx Jul 31, 2013

Choose a reason for hiding this comment

TomMalbran Aug 1, 2013

Choose a reason for hiding this comment

TomMalbran commented Aug 1, 2013

jasonsanjose commented Aug 1, 2013

TomMalbran commented Aug 1, 2013

TomMalbran commented Aug 1, 2013

jasonsanjose commented Aug 1, 2013

jasonsanjose commented Aug 1, 2013

jasonsanjose commented Aug 1, 2013

TomMalbran commented Aug 1, 2013

jasonsanjose commented Aug 1, 2013

TomMalbran commented Aug 1, 2013

jasonsanjose commented Aug 1, 2013

TomMalbran commented Aug 1, 2013

TomMalbran commented Aug 1, 2013

jasonsanjose commented Aug 1, 2013

TomMalbran commented Aug 1, 2013