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

LiveDevelopmentMultiBrowser unit test issues #10206

Open
peterflynn opened this issue Dec 17, 2014 · 9 comments · Fixed by #10285
Open

LiveDevelopmentMultiBrowser unit test issues #10206

peterflynn opened this issue Dec 17, 2014 · 9 comments · Fixed by #10285

Comments

@peterflynn
Copy link
Member

1) The Jasmine test-runner window always logs an exception when it is first loaded, before any tests are run: [console messages hidden]

(because bootstrap-twipsy-mod is not loaded in the Jasmine window) Fixed in #10285

2) Several LiveDevelopmentMultiBrowser tests fail for me with "timeout: timed out after 5000 msec waiting for livedevelopment.done.opened" (usually 2-3 tests fail per run).

3) The console is littered with error messages after running this test suite:

 [testWindow]  [node-error 10:14:11 AM] nodeSocketTransport: Socket closed, but couldn't locate client SpecRunnerUtils.js:543
runs.forEach._testWindow.console.(anonymous function) SpecRunnerUtils.js:543
handleLogEvent src/extensions/default/DebugCommands/NodeDebugUtils.js:114
trigger EventDispatcher.js:222
triggerWithArray EventDispatcher.js:260
NodeConnection._receive NodeConnection.js:479

(twice)

[testWindow]  Exception in 'base:log' listener on NodeConnection {domains: Object, _registeredModules: Array[0], _pendingInterfaceRefreshDeferreds: Array[0], _pendingCommandDeferreds: Array[0], _autoReconnect: true…}
TypeError: Cannot read property 'console' of null
    at Console.runs.forEach._testWindow.console.(anonymous function) [as log] (file:///C:/code/Brackets/brackets-app/brackets/test/spec/SpecRunnerUtils.js:544:53)
    at handleLogEvent (file:///C:/code/Brackets/brackets-app/brackets/src/extensions/default/DebugCommands/NodeDebugUtils.js:117:21)
    at NodeConnection.trigger (file:///C:/code/Brackets/brackets-app/brackets/src/utils/EventDispatcher.js:222:40)
    at Object.triggerWithArray (file:///C:/code/Brackets/brackets-app/brackets/src/utils/EventDispatcher.js:260:28)
    at NodeConnection._receive (file:///C:/code/Brackets/brackets-app/brackets/src/utils/NodeConnection.js:479:29)

(13 times - various listeners all with the same TypeError)

Even if these are caused by the timeouts above, it suggests the code does not fail gracefully in the event of a connection timeout.

4) One time this test suite seemed to get stuck with the Node process hanging onto file handles in the brackets/test/temp folder, which caused many other unit tests to fail since they were unable to clear that folder. Restating the Node process released the file handles. Depending on how easy it is to hit this failure case, we may want to do more to increase robustness...

@busykai
Copy link
Contributor

busykai commented Dec 17, 2014

CC @sebaslv

@busykai busykai self-assigned this Dec 17, 2014
@peterflynn
Copy link
Member Author

There are also tons of non-error console status messages -- the comments near the end of the original PR (e.g. #10010 (diff)) refer to only two log statements, one of which has been removed... but the amount of stuff I see in the console is far beyond that...

@peterflynn
Copy link
Member Author

I see 73 references to console.log in the diff. It looks like a few of those are used only in error cases (where it's fine to write stuff to the console, though those should probably change to .warn or .error)... But many of them look like they'll spam the console repeatedly during the normal course of operations as well.

@busykai
Copy link
Contributor

busykai commented Dec 17, 2014

Yeah, I know. These are two help debugging, supposedly. We will be cleaning them up. The current set of test is badly orchestrated (which causes exceptions) and, actually, not re-runnable.

@peterflynn
Copy link
Member Author

We could spin the console spam off as a separate bug if desired though.

Also -- I'm not sure how LiveDevelopmentMultiBrowser causes the first issue listed above, just looking at the diff, but if I go back to before #10010 landed (e.g. commit b8f92c1^ = 04efd92), the error goes away... so it seems likely to be caused by the new impl.

@busykai
Copy link
Contributor

busykai commented Dec 17, 2014

Let us finish up a mail with the TODO list as we see it, we will then create more or less complete list of things (issues and stories) to track the progress of this.

FWIW, if this implementation is not enabled, there's only one extra log message on the console (for a casual user).

@busykai
Copy link
Contributor

busykai commented Dec 27, 2014

@peterflynn, see #10285 for the issue 1 on your list.

Also, I'm curious, what's your default browser? I'm using FF Aurora, the tests run OK for me (not to say they don't have all these issues).

@busykai
Copy link
Contributor

busykai commented Jan 15, 2015

@peterflynn, could you try the current master to see if all of your issues are gone? If not, we'll re-open this. Also, still need the info on the default browser you're using.

@redmunds
Copy link
Contributor

Re-opening. I didn't mean to close this -- it must have auto-closed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants