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

Fix failing TestMaintainanceSignal #36

Merged
merged 1 commit into from
Dec 7, 2015

Conversation

tgross
Copy link
Contributor

@tgross tgross commented Dec 5, 2015

Trying to test the receiving of signals is subject to race conditions that cause different test failures in different environments but that aren't going to cause problems in realistic production environments
where signals we're interested in are either idempotent (SIGTERM) or can be expected to be received one-at-a-time (SIGUSR1). Integration tests verify our behavior is correct.

This PR factors out the tests that cover the signal handling code so that we can test their logic without dealing with coercing the async signals into a synchronous test model.

This code, before rebase, passed the TravisCI build here: https://travis-ci.org/tgross/containerbuddy/builds/95072309
This code, after rebase, passed the Travis CI build here: https://travis-ci.org/tgross/containerbuddy/builds/95073115
(And of course the PR build has now passed as well)

cc @justenwalker @misterbisson

Trying to test the receiving of signals is subject to race conditions
that cause different test failures in different environments but that
aren't going to cause problems in realistic production environments
where signals we're interested in are either idempotent (SIGTERM) or
can be expected to be received one-at-a-time (SIGUSR1). Integration
tests verify our behavior is correct.

This commit factors out the tests that cover the signal handling code
so that we can test their logic without dealing with coercing the
async signals into a synchronous test model.
@tgross
Copy link
Contributor Author

tgross commented Dec 5, 2015

This was my mistake, but could you change the 0 to a 2? The message is wrong; but no functional impact on the test.

Done. I rebased that minor fix onto this PR, so I lost your other comments from tgross@94e00fd so I'll address them here.

So is this the actual fix here? Adding a timeout for waiting?

The timeout prevents a case where if we screw up the lock on some hypothetical future handler and that deadlocks the test run (I ran into this while debugging). That bit was borrowed from the golang tests: https://golang.org/src/os/signal/signal_test.go

The "actual fix" is that we're not waiting for the signals in TestMaintenanceSignal at all anymore, which means we can verify the toggle is working without worrying about trying to figure out whether the signal has been handled yet (which requires we have synchronous knowledge about the async signal).

@justenwalker
Copy link
Contributor

The "actual fix" is that we're not waiting for the signals in TestMaintenanceSignal at all anymore, which means we can verify the toggle is working without worrying about trying to figure out whether the signal has been handled yet (which requires we have synchronous knowledge about the async signal).

I see what you are saying, the signals test has been refactored into its own test of just the signals.
And the previously failing tests are just testing the behavior, not the signal handling.

I like it 👍

tgross added a commit that referenced this pull request Dec 7, 2015
Fix failing TestMaintainanceSignal
@tgross tgross merged commit 9de330d into TritonDataCenter:master Dec 7, 2015
@tgross tgross deleted the signal_test_refactor branch April 4, 2017 15:46
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

Successfully merging this pull request may close these issues.

None yet

2 participants