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

Show output from tests when process is killed via CTRL+C #66

Closed
wants to merge 1 commit into from
Closed

Show output from tests when process is killed via CTRL+C #66

wants to merge 1 commit into from

Conversation

lm
Copy link
Contributor

@lm lm commented Dec 28, 2013

Runner now checks for SIGINT and after receiving signal invokes output handlers and returns.

@fprochazka
Copy link
Contributor

I've tested it and it works flawlesly! Great work! 👍

$ ./tests/run.sh -j 5 ./tests/
PHP 5.5.7 | 'php-cgi' -n -c 'tests/php.ini-unix' | 5 threads

.......^C


OK (7 tests, 8.0 seconds)

if ($this->handleInterrupt) {
$this->interrupted = FALSE;
pcntl_signal(SIGINT, function() { $this->interrupted = TRUE; });
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a good practice to add pcntl_signal(SIGINT, SIG_DFL); into handler body.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And $this does not exists in closure in older PHP.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added pcntl_signal(SIGINT, SIG_DFL); to the end of run() and removed $this from closure.

@fprochazka
Copy link
Contributor

This is probably expected

$ ./tests/run.sh -j 5 ./tests/
PHP 5.5.7 | 'php-cgi' -n -c 'tests/php.ini-unix' | 5 threads

.......^CF

-- FAILED: ApiFoodController. | ApiFoodController.phpt ['testResponse']
   Exited with error code -1 (expected 0)


FAILURES! (8 tests, 1 failures, 8.1 seconds)

But the already running tests should be thrown away because they didn't in fact failed, they were killed.

@fprochazka
Copy link
Contributor

@lm much better!

@lm
Copy link
Contributor Author

lm commented Dec 28, 2013

Fixed (assessing of killed jobs).

@@ -85,11 +97,21 @@ public function run()
}

foreach ($running as $key => $job) {
$this->dispatchSignal();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why dispatch there? Would not already finished job pass?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point it's not clear if job completed or was killed by signal, so is better to skip result check.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I see.

@fprochazka
Copy link
Contributor

@dg @milo ping?

Just FYI: I'm using it and it works flawlesly.

@milo
Copy link
Member

milo commented Jan 11, 2014

I like it too. After traveling, I'll work on it.

@milo
Copy link
Member

milo commented Jan 12, 2014

Merged 8021168

@milo milo closed this Jan 12, 2014
@hrach
Copy link

hrach commented Jan 12, 2014

👍

@fprochazka
Copy link
Contributor

Love it!

@fprochazka fprochazka mentioned this pull request Mar 7, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants