Page MenuHomePhabricator

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

Authored by abrachet on Mar 4 2020, 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.Mar 4 2020, 3:02 PM
MaskRay added inline comments.Mar 4 2020, 4:00 PM
libc/utils/testutils/ExecuteFunction.h
25

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.Mar 4 2020, 4:57 PM
abrachet added inline comments.
libc/utils/testutils/ExecuteFunction.h
25

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.
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.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