This is an archive of the discontinued LLVM Phabricator instance.

[libc] [UnitTest] Create death tests
ClosedPublic

Authored by abrachet on Feb 14 2020, 11:06 PM.

Details

Summary

This patch adds EXPECT_EXITS and EXPECT_DEATH macros for testing exit codes and deadly signals. They are less convoluted than their analogs in GTEST and don't have matchers but just take an int for either the exit code or the signal respectively. Nor do they have any regex match against the stdout/stderr of the child process.

Diff Detail

Event Timeline

abrachet created this revision.Feb 14 2020, 11:06 PM

Stupidly I didn't branch against D74528, so I do not have an example test in this patch, but the death test for raise would look like EXPECT_DEATH([] { __llvm_libc::raise(SIGKILL); }, SIGKILL);

Overall looks good. But, one comment about eliminating the platform specificity from the change as proposed.

libc/utils/UnitTest/Test.cpp
14

Can we build a generic forking utility and put it in the testutils library I have proposed in my other patch? This will help us keep the unittest implementation from pulling in and using platform specific pieces.

libc/utils/UnitTest/Test.h
41

Instead of putting them in the internal namespace, can we nest them inside of of the Test class? It seems a bit odd for an internal class to be part of the public API of the Test class.

abrachet updated this revision to Diff 246064.Feb 22 2020, 1:34 AM
  • Create testutils directory modeled from D74652.
  • Move platform specific process creation into Unix specific file.
  • Add death test for raise
abrachet updated this revision to Diff 246093.Feb 22 2020, 5:28 PM

Remove extraneous semicolon.

gchatelet added inline comments.Feb 24 2020, 1:04 AM
libc/utils/testutils/ExecuteFunction.h
35 ↗(On Diff #246093)

[nit] Add new line

sivachandra added inline comments.Feb 24 2020, 9:12 AM
libc/utils/UnitTest/CMakeLists.txt
7 ↗(On Diff #246093)

This shouldn't be required as we want to include like #include "utils/testutils/...".

libc/utils/UnitTest/Test.cpp
11

Use `utils/...' as suggested above.

256

Can we avoid calling strsignal in this file? May be add a signalAsString helper function in ExecuteFunction.h

libc/utils/UnitTest/Test.h
12

Use utils/... as suggested above.

libc/utils/testutils/ExecuteFunction.h
21 ↗(On Diff #246093)

Call this struct ProcessStatus as it is actually that? Otherwise, it is easy to confuse with function return value.

29 ↗(On Diff #246093)

Name the function invokeInSubprocess?

abrachet updated this revision to Diff 246300.Feb 24 2020, 2:10 PM
  • Include using "utils/" prefix,
  • Create strsignal wrapper, signalAsString
  • Rename InvokeResult to ProcessStatus
  • Rename invokeFunction to invokeInSubprocess
  • Added missing newline
abrachet marked 8 inline comments as done.Feb 24 2020, 2:11 PM
sivachandra accepted this revision.Feb 24 2020, 2:38 PM
sivachandra marked an inline comment as done.
  • Include using "utils/" prefix,
  • Create strsignal wrapper, signalAsString
  • Rename InvokeResult to ProcessStatus
  • Rename invokeFunction to invokeInSubprocess
  • Added missing newline

Thanks for listing the changes.

libc/utils/UnitTest/Test.h
10

Sigh! Thanks!

This revision is now accepted and ready to land.Feb 24 2020, 2:38 PM
abrachet marked an inline comment as done.Feb 24 2020, 2:53 PM
abrachet added inline comments.
libc/utils/UnitTest/Test.h
10

I forget it all the time too :)

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 24 2020, 2:57 PM

Can you try building this with shared library ?

FAILED: lib/liblibc_test_utils.so 
: && /redacted/build-llvm/clang/bin/clang++ -fPIC -fPIC -fvisibility-inlines-hidden -Werror -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -g  -Wl,-z,defs -Wl,-z,nodelete -fuse-ld=lld -Wl,--color-diagnostics -shared -Wl,-soname,liblibc_test_utils.so -o lib/liblibc_test_utils.so projects/libc/utils/testutils/CMakeFiles/libc_test_utils.dir/ExecuteFunctionUnix.cpp.o   && :
ld.lld: error: undefined symbol: llvm::outs()
>>> referenced by ExecuteFunctionUnix.cpp:35 (/redacted/git/llvm-project/libc/utils/testutils/ExecuteFunctionUnix.cpp:35)
>>>               projects/libc/utils/testutils/CMakeFiles/libc_test_utils.dir/ExecuteFunctionUnix.cpp.o:(__llvm_libc::testutils::invokeInSubprocess(__llvm_libc::testutils::FunctionCaller*))

ld.lld: error: undefined symbol: llvm::errs()
>>> referenced by ExecuteFunctionUnix.cpp:36 (/redatced/git/llvm-project/libc/utils/testutils/ExecuteFunctionUnix.cpp:36)
>>>               projects/libc/utils/testutils/CMakeFiles/libc_test_utils.dir/ExecuteFunctionUnix.cpp.o:(__llvm_libc::testutils::invokeInSubprocess(__llvm_libc::testutils::FunctionCaller*))

ld.lld: error: undefined symbol: llvm::raw_ostream::flush_nonempty()
>>> referenced by raw_ostream.h:157 (/redacted/git/llvm-project/llvm/include/llvm/Support/raw_ostream.h:157)
>>>               projects/libc/utils/testutils/CMakeFiles/libc_test_utils.dir/ExecuteFunctionUnix.cpp.o:(llvm::raw_ostream::flush())

ld.lld: error: undefined symbol: llvm::EnableABIBreakingChecks
>>> referenced by ExecuteFunctionUnix.cpp
>>>               projects/libc/utils/testutils/CMakeFiles/libc_test_utils.dir/ExecuteFunctionUnix.cpp.o:(llvm::VerifyEnableABIBreakingChecks)
clang: error: linker command failed with exit code 1 (use -v to see invocation)