Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[CLOSED] Reuse test windows during integration suites #4270

Open
core-ai-bot opened this issue Aug 29, 2021 · 16 comments
Open

[CLOSED] Reuse test windows during integration suites #4270

core-ai-bot opened this issue Aug 29, 2021 · 16 comments

Comments

@core-ai-bot
Copy link
Member

Issue by jasonsanjose
Wednesday Jul 31, 2013 at 22:26 GMT
Originally opened as adobe/brackets#4618

  • 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 included the following code: https://github.com/adobe/brackets/pull/4618/commits
@core-ai-bot
Copy link
Member Author

Comment by jasonsanjose
Wednesday Jul 31, 2013 at 22:26 GMT

@TomMalbran want to review?

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Thursday Aug 01, 2013 at 00:09 GMT

@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?

@core-ai-bot
Copy link
Member Author

Comment by jasonsanjose
Thursday Aug 01, 2013 at 18:32 GMT

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)

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Thursday Aug 01, 2013 at 19:41 GMT

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.

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Thursday Aug 01, 2013 at 20:47 GMT

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.

@core-ai-bot
Copy link
Member Author

Comment by jasonsanjose
Thursday Aug 01, 2013 at 20:59 GMT

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.

@core-ai-bot
Copy link
Member Author

Comment by jasonsanjose
Thursday Aug 01, 2013 at 22:16 GMT

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.

@core-ai-bot
Copy link
Member Author

Comment by jasonsanjose
Thursday Aug 01, 2013 at 22:20 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Thursday Aug 01, 2013 at 22:24 GMT

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.

@core-ai-bot
Copy link
Member Author

Comment by jasonsanjose
Thursday Aug 01, 2013 at 22:35 GMT

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.

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Thursday Aug 01, 2013 at 23:00 GMT

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?

@core-ai-bot
Copy link
Member Author

Comment by jasonsanjose
Thursday Aug 01, 2013 at 23:07 GMT

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.

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Thursday Aug 01, 2013 at 23:14 GMT

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.

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Thursday Aug 01, 2013 at 23:14 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by jasonsanjose
Thursday Aug 01, 2013 at 23:29 GMT

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.

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Thursday Aug 01, 2013 at 23:43 GMT

Awesome :)

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@core-ai-bot