Page MenuHomePhabricator

[libc] [UnitTest] Add timeout to death tests
ClosedPublic

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

Details

Summary

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.

Additionally:

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

Diff Detail

Event Timeline

abrachet created this revision.Wed, Mar 4, 3:02 PM
MaskRay added inline comments.Wed, Mar 4, 4:00 PM
libc/utils/testutils/ExecuteFunction.h
25–40

I think for new projects, we don't have to use VariableName. We should use the ideal case variableName.

https://llvm.org/docs/Proposals/VariableNames.html 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.Wed, Mar 4, 4:57 PM
abrachet added inline comments.
libc/utils/testutils/ExecuteFunction.h
25–40

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.Wed, Mar 4, 8:33 PM

Made variables added by this patch start with lower case

PaulkaToast marked an inline comment as done.Thu, Mar 5, 9:42 AM
PaulkaToast added inline comments.
libc/utils/testutils/ExecuteFunctionUnix.cpp
64

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.Thu, Mar 5, 11:01 AM

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

PaulkaToast accepted this revision.EditedMon, Mar 9, 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.Mon, Mar 9, 7:06 PM
sivachandra accepted this revision.Wed, Mar 11, 7:18 AM
abrachet updated this revision to Diff 249823.Wed, Mar 11, 8:57 PM

Add comment.

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