after is not getting called when the test process is interrupted by hitting Ctrl + C #516

Closed
c4milo opened this Issue Jul 19, 2012 · 26 comments

Comments

Projects
None yet
3 participants
@c4milo

c4milo commented Jul 19, 2012

@visionmedia what do you think about catching SIGTERM, SIGINT and SIGHUP, running the after callback function to clean things up in the test?

@c4milo c4milo closed this Jul 19, 2012

@c4milo c4milo reopened this Jul 19, 2012

@tj

This comment has been minimized.

Show comment Hide comment
@tj

tj Jul 19, 2012

Contributor

hmmm in general -1 but what's the cleanup doing that is so crucial? i know it doesnt fit every case but i usually clean out the db in a before / beforeEach etc so even failures are fine

Contributor

tj commented Jul 19, 2012

hmmm in general -1 but what's the cleanup doing that is so crucial? i know it doesnt fit every case but i usually clean out the db in a before / beforeEach etc so even failures are fine

@tj

This comment has been minimized.

Show comment Hide comment
@tj

tj Jul 19, 2012

Contributor

actually i almost never use after / afterEach

Contributor

tj commented Jul 19, 2012

actually i almost never use after / afterEach

@c4milo

This comment has been minimized.

Show comment Hide comment
@c4milo

c4milo Jul 19, 2012

well, we are seeding the database only once in the before callback, then removing the data in the after callback function. Tests run faster than doing it per every test in the suite.

c4milo commented Jul 19, 2012

well, we are seeding the database only once in the before callback, then removing the data in the after callback function. Tests run faster than doing it per every test in the suite.

@tj

This comment has been minimized.

Show comment Hide comment
@tj

tj Jul 19, 2012

Contributor

gotcha, why not add another before() and clear it first? that's the approach I usually take, come to think of it I dont think I've ever used after / afterEach

Contributor

tj commented Jul 19, 2012

gotcha, why not add another before() and clear it first? that's the approach I usually take, come to think of it I dont think I've ever used after / afterEach

@c4milo

This comment has been minimized.

Show comment Hide comment
@c4milo

c4milo Jul 19, 2012

¯(°_o)/¯

c4milo commented Jul 19, 2012

¯(°_o)/¯

@tj

This comment has been minimized.

Show comment Hide comment
@tj

tj Jul 19, 2012

Contributor

hahaha that's sweet

Contributor

tj commented Jul 19, 2012

hahaha that's sweet

@c4milo

This comment has been minimized.

Show comment Hide comment
@c4milo

c4milo Jul 19, 2012

my specs were looking so natural until a hit this wall. I can use before to remove the data and then reload it again but it doesn't feel right to me that after doesn't get called upon any sort of test interruption :dissapointed:

c4milo commented Jul 19, 2012

my specs were looking so natural until a hit this wall. I can use before to remove the data and then reload it again but it doesn't feel right to me that after doesn't get called upon any sort of test interruption :dissapointed:

@tj

This comment has been minimized.

Show comment Hide comment
@tj

tj Jul 19, 2012

Contributor

IMO it just makes more sense than trying to recover from every possible fail point, im not 100% against it if it was an option but it's sort of a non-problem, I agree though that most people will be doing it this way and that it feels more natural

Contributor

tj commented Jul 19, 2012

IMO it just makes more sense than trying to recover from every possible fail point, im not 100% against it if it was an option but it's sort of a non-problem, I agree though that most people will be doing it this way and that it feels more natural

@c4milo

This comment has been minimized.

Show comment Hide comment
@c4milo

c4milo Jul 19, 2012

I'm going to implement remove if it exists in my fixture loading functions and leave the after callback anyways because I know you are going to reconsider this :P

c4milo commented Jul 19, 2012

I'm going to implement remove if it exists in my fixture loading functions and leave the after callback anyways because I know you are going to reconsider this :P

@tj

This comment has been minimized.

Show comment Hide comment
@tj

tj Jul 19, 2012

Contributor

hahaha, nah I totally get it, just thinking on it. we need some more people in here that use after hooks, because im not totally convinced that they should exist

Contributor

tj commented Jul 19, 2012

hahaha, nah I totally get it, just thinking on it. we need some more people in here that use after hooks, because im not totally convinced that they should exist

@c4milo

This comment has been minimized.

Show comment Hide comment
@c4milo

c4milo Jul 19, 2012

you know they exist in every single respetable BDD/TDD/xUnit framework, that's why you designed it like that in mocha. It just needs a small tweak heheh

c4milo commented Jul 19, 2012

you know they exist in every single respetable BDD/TDD/xUnit framework, that's why you designed it like that in mocha. It just needs a small tweak heheh

@tj

This comment has been minimized.

Show comment Hide comment
@tj

tj Jul 19, 2012

Contributor

yup, doesn't mean they really make sense though. A test can fail in so many different ways, if the machine shuts down you have bad state, if you get SIGKILL you have bad state etc

Contributor

tj commented Jul 19, 2012

yup, doesn't mean they really make sense though. A test can fail in so many different ways, if the machine shuts down you have bad state, if you get SIGKILL you have bad state etc

@c4milo

This comment has been minimized.

Show comment Hide comment
@c4milo

c4milo Jul 19, 2012

indeed, but those are exceptional situations, hitting Ctrl + C to terminate the runner in development time is fairly common.

c4milo commented Jul 19, 2012

indeed, but those are exceptional situations, hitting Ctrl + C to terminate the runner in development time is fairly common.

@tj

This comment has been minimized.

Show comment Hide comment
@tj

tj Jul 19, 2012

Contributor

yeah i thing SIGINT is reasonable

Contributor

tj commented Jul 19, 2012

yeah i thing SIGINT is reasonable

@c4milo

This comment has been minimized.

Show comment Hide comment
@c4milo

c4milo Jul 19, 2012

c4milo commented Jul 19, 2012

@tj

This comment has been minimized.

Show comment Hide comment
@tj

tj Jul 19, 2012

Contributor

hahahahaha. ill try it in a min

Contributor

tj commented Jul 19, 2012

hahahahaha. ill try it in a min

@tj

This comment has been minimized.

Show comment Hide comment
@tj

tj Jul 19, 2012

Contributor

so it does work fine, but I think this will have edge-cases regardless, if you SIGINT during one of the after hooks then you're fucked anyway

Contributor

tj commented Jul 19, 2012

so it does work fine, but I think this will have edge-cases regardless, if you SIGINT during one of the after hooks then you're fucked anyway

@c4milo

This comment has been minimized.

Show comment Hide comment
@c4milo

c4milo Jul 19, 2012

hm, I was expecting it to call after callbacks upon SIGINT and then exit the process. Isn't that how it works?

c4milo commented Jul 19, 2012

hm, I was expecting it to call after callbacks upon SIGINT and then exit the process. Isn't that how it works?

@tj

This comment has been minimized.

Show comment Hide comment
@tj

tj Jul 19, 2012

Contributor

that part works fine, but if what's hanging is an after hook then you're screwed

Contributor

tj commented Jul 19, 2012

that part works fine, but if what's hanging is an after hook then you're screwed

@tj

This comment has been minimized.

Show comment Hide comment
@tj

tj Jul 19, 2012

Contributor

or if you just happen to SIGINT at the wrong time

Contributor

tj commented Jul 19, 2012

or if you just happen to SIGINT at the wrong time

@c4milo

This comment has been minimized.

Show comment Hide comment
@c4milo

c4milo Jul 19, 2012

agreed, it minimizes the chance to screw things up anyway. Do you feel like pushing that into master or you're not entirely sure yet?

c4milo commented Jul 19, 2012

agreed, it minimizes the chance to screw things up anyway. Do you feel like pushing that into master or you're not entirely sure yet?

@c4milo

This comment has been minimized.

Show comment Hide comment
@c4milo

c4milo Jul 19, 2012

From my experience the most common scenario is the test hanging for some reason, the after function is usually pretty straight forward.

c4milo commented Jul 19, 2012

From my experience the most common scenario is the test hanging for some reason, the after function is usually pretty straight forward.

@tj

This comment has been minimized.

Show comment Hide comment
@tj

tj Jul 19, 2012

Contributor

im not feelin it so far, mostly because then we are kind of promising something that isn't really possible, the only thing you could do is flat-out ignore SIGINT when after hooks are being fired, but then if you are really hanging you'll have to kill(1) it which is not any more convenient

Contributor

tj commented Jul 19, 2012

im not feelin it so far, mostly because then we are kind of promising something that isn't really possible, the only thing you could do is flat-out ignore SIGINT when after hooks are being fired, but then if you are really hanging you'll have to kill(1) it which is not any more convenient

@c4milo

This comment has been minimized.

Show comment Hide comment
@c4milo

c4milo Jul 19, 2012

fair enough, let's leave it open for now until we clear things up a bit more. I wonder how other test frameworks deal with this though.

c4milo commented Jul 19, 2012

fair enough, let's leave it open for now until we clear things up a bit more. I wonder how other test frameworks deal with this though.

@tj

This comment has been minimized.

Show comment Hide comment
@tj

tj Jul 19, 2012

Contributor

yup it's still something to consider

Contributor

tj commented Jul 19, 2012

yup it's still something to consider

@jbnicolai

This comment has been minimized.

Show comment Hide comment
@jbnicolai

jbnicolai Jun 7, 2015

Contributor

Issue has been inactive for years, and I feel @tj was completely right in his doubt about adding the feature. Closing for now, happy to re-open if someone feels like this is still a problem.

Contributor

jbnicolai commented Jun 7, 2015

Issue has been inactive for years, and I feel @tj was completely right in his doubt about adding the feature. Closing for now, happy to re-open if someone feels like this is still a problem.

@jbnicolai jbnicolai closed this Jun 7, 2015

@MicahZoltu MicahZoltu referenced this issue in ethereumjs/ethereumjs-stub-rpc-server Mar 27, 2017

Open

EADDRINUSE after force-quitting stub-rpc-server #2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment