Skip to content

Use an error status instead of nil if we're missing a worker status code#70

Merged
aroben merged 6 commits intotmm1:masterfrom
bhuga:resilient-against-nil-exit-status
Jan 17, 2017
Merged

Use an error status instead of nil if we're missing a worker status code#70
aroben merged 6 commits intotmm1:masterfrom
bhuga:resilient-against-nil-exit-status

Conversation

@bhuga
Copy link
Collaborator

@bhuga bhuga commented Dec 23, 2016

Workers missing an exit status are an error. This can happen if a worker's process is killed out-of-band. Currently, summarize will explode on the % call if any of these values are nil.

This doesn't seem too tough to write a test for, but I'm not sure where to put it. Should we copy one of the suites, say minitest5, and have a suite for test-queue features as opposed to end-to-end tests for each test framework?

Copy link
Collaborator

@aroben aroben left a comment

Choose a reason for hiding this comment

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

This doesn't seem too tough to write a test for, but I'm not sure where to put it. Should we copy one of the suites, say minitest5, and have a suite for test-queue features as opposed to end-to-end tests for each test framework?

A bunch of the minitest5 tests are already like this. It would definitely be nice to break them out to make it clearer that they are not specific to minitest5.

worker.end_time - worker.start_time,
worker.pid,
worker.status.exitstatus,
worker.status.exitstatus || 1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

It'd be nice to show the signal name if the process was killed by a signal. In fact, it looks like we could replace the whole pid %d exit %d portion of the string with worker.status.to_s, which gives us that same information including info about the signal.

@aroben
Copy link
Collaborator

aroben commented Dec 23, 2016

There are other uses of worker.status.exitstatus that probably need to guard against nil too. Could you take a look at those?

@bhuga
Copy link
Collaborator Author

bhuga commented Jan 16, 2017

Okay, I think the only other bad case is covered, and we now output more information when a process dies in an unexpected way. This is ready for another look.

Copy link
Collaborator

@aroben aroben left a comment

Choose a reason for hiding this comment

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

Looks great!

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.

2 participants