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.

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

testem doesn't close phantomjs v1.7.0 on OSX #92

Closed
opichals opened this issue Nov 22, 2012 · 12 comments
Closed

testem doesn't close phantomjs v1.7.0 on OSX #92

opichals opened this issue Nov 22, 2012 · 12 comments

Comments

@opichals
Copy link
Contributor

It is probably missing the phantom.exit() call after all-tests-results message was received? Could you please have a look?

console.log('Loading a web page');
var page = require('webpage').create();
var url = 'http://www.phantomjs.org/';
page.open(url, function (status) {
    //Page is loaded!
    phantom.exit();
});
$ killall phantomjs
$ testem ci -d --launch phantomjs --test_page html/test/xhr.html?autoRun=1
# Launching PhantomJS
# ...
TAP version 13
ok 1 - PhantomJS should test marks and headers during authentication redirection
ok 2 - PhantomJS should call decision function when response was successfull
ok 3 - PhantomJS shouldn't call decision function after aborting poller


1..3
# tests 3
# pass  3

# ok
$ ps
  PID TTY           TIME CMD
13004 ttys000    0:06.83 -bash
25036 ttys000    0:00.72 /Users/test/bin/osx/phantomjs-1.7.0-macosx/bin/phantomjs /Users/test/npm/node_modules/testem/assets/ph...
28265 ttys001    0:01.04 -bash
57700 ttys002    0:00.14 -bash
$ testem ci -d --launch phantomjs --test_page html/test/xhr.html?autoRun=1
# Launching PhantomJS
# ...
TAP version 13
ok 1 - PhantomJS should test marks and headers during authentication redirection
ok 2 - PhantomJS should call decision function when response was successfull
ok 3 - PhantomJS shouldn't call decision function after aborting poller


1..3
# tests 3
# pass  3

# ok
$ ps
  PID TTY           TIME CMD
13004 ttys000    0:06.84 -bash
25036 ttys000    0:00.97 /Users/test/bin/osx/phantomjs-1.7.0-macosx/bin/phantomjs /Users/test/npm/node_modules/testem/assets/ph...
25043 ttys000    0:00.51 /Users/test/bin/osx/phantomjs-1.7.0-macosx/bin/phantomjs /Users/test/npm/node_modules/testem/assets/ph...
28265 ttys001    0:01.04 -bash
57700 ttys002    0:00.14 -bash
$ 
@airportyh
Copy link
Collaborator

Killing phantomjs is problematic sometimes. But it happens only very sporadically for me so it's hard to test. But it looks like it's happening every time for you. Can you share a gist with me so I can test with the exact same setup?

@devonhumes
Copy link

For me the issue happens every time an error kills the session. For example, if I have invalid JSON sytax in the testem.json file, then the testem process crashes and PhantomJS does not die.

@airportyh
Copy link
Collaborator

Ah, I see. I'll try that.

@opichals
Copy link
Contributor Author

Here is a gist which exhibits the behavior https://gist.github.com/4160751 . It doesn't depend on the particular ok/error results.

@airportyh
Copy link
Collaborator

Thanks, I downloaded your gist, but still can't reproduce what you are seeing though. However, I make an attempt at a fix by speculation. Can you try testing this branch and see if that fixes it?

https://github.com/airportyh/testem/tree/kill_phantom

@opichals
Copy link
Contributor Author

Thanks for the branch! I just tested it and found it is still not working. Still the same effect.

But at least I now get an idea on where to start and perhaps try to debug this later.

@opichals
Copy link
Contributor Author

Tested the problem with phantomjs 1.5.0 and that version doesn't exhibit the problem.

The issue goes away if do phantom.exit() inside the launcher script. So it would be nice to be able to deliver the socket communication to the phantom script and do phantom.exit() properly. Perhaps by wrapping socket.emit() with something that communicates via onConsoleMessage() with phantomjs which would be able to shut properly.

@airportyh
Copy link
Collaborator

This issue is probably related https://groups.google.com/forum/?fromgroups=#!topic/phantomjs/kaEzJ45VS0c

Ideally, I would like to avoid writing phantomjs specific code just for this, but if it's the only way...

@opichals could you experiment with your approach and see if it works?

@opichals
Copy link
Contributor Author

opichals commented Dec 3, 2012

Thanks for the phantomjs discussion link. It looks like they eventually found a reliable way. Will see if that helps.

In any case it would be nice if testem could shut phantom properly. However like you wrote, having a phantomjs specific launcher doesn't sound as a clear solution and phantomjs script cannot probably communicate with the page in other than console message terms... on the other hand having a console.log in emit() method of the tie socket wrapper doesn't sounds that bad, or does it?

@opichals
Copy link
Contributor Author

opichals commented Dec 3, 2012

With a master build of phantomjs the original https://gist.github.com/4160751 passes without leaving phantomjs instance running. So consider that part solved.

Now I have updated the test page to simulate a test 'failure' (pull the gist again for simulation). And that one results into the following output (while leaving a phantomjs process running):

71143 ttys000    0:00.00 /bin/sh -c ps | grep phantomjs
71145 ttys000    0:00.00 grep phantomjs
testem --version
0.2.41
testem ci --launch phantomjs --test_page ./*.html
# Launching PhantomJS
# .

/Users/standa/Sites/gdc/client/node_modules/testem/lib/ci_mode_app.js:220
                var item = test.get('items').filter(function(i){
                                             ^
TypeError: Cannot call method 'filter' of undefined
    at EventEmitter.App.outputTap (/Users/standa/Sites/gdc/client/node_modules/testem/lib/ci_mode_app.js:220:46)
    at Array.forEach (native)
    at Function._.each._.forEach (/Users/standa/Sites/gdc/client/node_modules/testem/node_modules/underscore/underscore.js:79:11)
    at _.each.Collection.(anonymous function) [as forEach] (/Users/standa/Sites/gdc/client/node_modules/testem/node_modules/backbone/backbone.js:859:24)
    at EventEmitter.App.outputTap (/Users/standa/Sites/gdc/client/node_modules/testem/lib/ci_mode_app.js:208:30)
    at App.runAllTheTests (/Users/standa/Sites/gdc/client/node_modules/testem/lib/ci_mode_app.js:156:22)
    at finishLine (/Users/standa/Sites/gdc/client/node_modules/testem/lib/race.js:8:14)
    at EventEmitter.<anonymous> (/Users/standa/Sites/gdc/client/node_modules/testem/lib/ci_mode_app.js:186:21)
    at EventEmitter.g (events.js:193:14)
    at EventEmitter.emit (events.js:123:20)

@airportyh
Copy link
Collaborator

@opichals I fixed this. Could you retest? It will still throw this exception, but the phantom process should be cleaned up.

@opichals
Copy link
Contributor Author

opichals commented Dec 4, 2012

Confirmed, fixed. Thanks!!!

@opichals opichals closed this as completed Dec 4, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants