Page MenuHomePhabricator

[libc] [UnitTest] Add timeout to death tests

Authored by abrachet on Mar 4 2020, 3:02 PM.



This patch adds a timeout of 500ms to death tests. As we add multithreaded code and locks, deadlocks become more likely so timeout will be useful.


  • Better error handling in invokeSubprocess
  • Makes ProcessStatus's methods const

Diff Detail

Event Timeline

abrachet created this revision.Mar 4 2020, 3:02 PM
MaskRay added inline comments.Mar 4 2020, 4:00 PM

I think for new projects, we don't have to use VariableName. We should use the ideal case variableName. Everyone agrees that the current VariableName convention is broken. People have concerns with existing code base because renaming can clutter up the history. But for a new project, there is simply not meaningful to start with VariableName.

abrachet marked an inline comment as done.Mar 4 2020, 4:57 PM
abrachet added inline comments.

Ok I will change the variables that I created in this patch. Did Rui upstream the tool he used to convert lld's code base so that I could use it here?

abrachet updated this revision to Diff 248387.Mar 4 2020, 8:33 PM

Made variables added by this patch start with lower case

PaulkaToast marked an inline comment as done.Mar 5 2020, 9:42 AM
PaulkaToast added inline comments.

I think it'd be useful to include the length of the timeout in the error message.
e.g "Process timed out after 500ms"

abrachet updated this revision to Diff 248554.Mar 5 2020, 11:01 AM

Added ProcessStatus::timedOut() and better error messages in Test::testProcessExits/Killed

PaulkaToast accepted this revision.EditedMar 9 2020, 7:06 PM

This looks ready to me.
nit: I'd maybe add some comments explaining the pipe trick since it might not be overtly obvious what's happening here on first glance. (:

This revision is now accepted and ready to land.Mar 9 2020, 7:06 PM
sivachandra accepted this revision.Mar 11 2020, 7:18 AM
abrachet updated this revision to Diff 249823.Mar 11 2020, 8:57 PM

Add comment.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2020, 9:07 PM