From dbf1cecac5584ad070abdc10a6452e92ee9c5e9a Mon Sep 17 00:00:00 2001 From: Alexey Samsonov Date: Mon, 17 Mar 2025 18:07:12 -0700 Subject: [PATCH 1/4] [libc] Create ErrnoCheckingTest harness to simplify errno tests. See the discussion in PR #131650 on why we need to clear the errno at the beginning of some tests, and outlining the various solutions. Introduce ErrnoCheckingTest base class and use it for unlink_test as an example. --- libc/test/UnitTest/CMakeLists.txt | 9 ++++ libc/test/UnitTest/ErrnoCheckingTest.h | 41 +++++++++++++++++++ libc/test/src/unistd/CMakeLists.txt | 3 +- libc/test/src/unistd/unlink_test.cpp | 18 ++++---- .../libc/test/UnitTest/BUILD.bazel | 11 +++++ .../libc/test/src/unistd/BUILD.bazel | 4 +- 6 files changed, 76 insertions(+), 10 deletions(-) create mode 100644 libc/test/UnitTest/ErrnoCheckingTest.h diff --git a/libc/test/UnitTest/CMakeLists.txt b/libc/test/UnitTest/CMakeLists.txt index 570bd600d1a43..b0a3a7431c222 100644 --- a/libc/test/UnitTest/CMakeLists.txt +++ b/libc/test/UnitTest/CMakeLists.txt @@ -188,3 +188,12 @@ add_header_library( libc.src.__support.StringUtil.error_to_string libc.src.errno.errno ) + +add_header_library( + ErrnoCheckingTest + HDRS + ErrnoCheckingTest.h + DEPENDS + libc.src.__support.common + libc.src.errno.errno +) diff --git a/libc/test/UnitTest/ErrnoCheckingTest.h b/libc/test/UnitTest/ErrnoCheckingTest.h new file mode 100644 index 0000000000000..c4583d09705fd --- /dev/null +++ b/libc/test/UnitTest/ErrnoCheckingTest.h @@ -0,0 +1,41 @@ +//===-- ErrnoCheckingTest.h ------------------------------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===---------------------------------------------------------------------===// + +#ifndef LLVM_LIBC_TEST_UNITTEST_ERRNOCHECKINGTEST_H +#define LLVM_LIBC_TEST_UNITTEST_ERRNOCHECKINGTEST_H + +#include "src/__support/macros/config.h" +#include "src/errno/libc_errno.h" +#include "test/UnitTest/Test.h" + +namespace LIBC_NAMESPACE_DECL { +namespace testing { + +// Provides a test fixture for tests that validate modifications of the errno. +// It clears out the errno at the beginning of the test (e.g. in case it +// contained the value pre-set by the system), and confirms it's still zero +// at the end of the test, forcing the test author to explicitly account for all +// non-zero values. +class ErrnoCheckingTest : public Test { +public: + void SetUp() override { + Test::SetUp(); + LIBC_NAMESPACE::libc_errno = 0; + } + + void TearDown() override { + ASSERT_ERRNO_SUCCESS(); + Test::TearDown(); + } +}; + +} // namespace testing +} // namespace LIBC_NAMESPACE_DECL + +#endif // LLVM_LIBC_TEST_UNITTEST_ERRNOCHECKINGTEST_H + diff --git a/libc/test/src/unistd/CMakeLists.txt b/libc/test/src/unistd/CMakeLists.txt index d1f3050e6cccf..d0f1214b6609e 100644 --- a/libc/test/src/unistd/CMakeLists.txt +++ b/libc/test/src/unistd/CMakeLists.txt @@ -353,10 +353,11 @@ add_libc_unittest( unlink_test.cpp DEPENDS libc.include.unistd - libc.src.errno.errno libc.src.fcntl.open libc.src.unistd.close libc.src.unistd.unlink + libc.test.UnitTest.ErrnoCheckingTest + libc.test.UnitTest.ErrnoSetterMatcher ) add_libc_unittest( diff --git a/libc/test/src/unistd/unlink_test.cpp b/libc/test/src/unistd/unlink_test.cpp index e1ffaab78147e..5921f4935001a 100644 --- a/libc/test/src/unistd/unlink_test.cpp +++ b/libc/test/src/unistd/unlink_test.cpp @@ -6,27 +6,29 @@ // //===----------------------------------------------------------------------===// -#include "src/errno/libc_errno.h" #include "src/fcntl/open.h" #include "src/unistd/close.h" #include "src/unistd/unlink.h" +#include "test/UnitTest/ErrnoCheckingTest.h" #include "test/UnitTest/ErrnoSetterMatcher.h" #include "test/UnitTest/Test.h" #include -TEST(LlvmLibcUnlinkTest, CreateAndUnlink) { - using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Succeeds; +using LlvmLibcUnlinkTest = LIBC_NAMESPACE::testing::ErrnoCheckingTest; +namespace ESM = LIBC_NAMESPACE::testing::ErrnoSetterMatcher; + +TEST_F(LlvmLibcUnlinkTest, CreateAndUnlink) { constexpr const char *FILENAME = "unlink.test"; auto TEST_FILE = libc_make_test_file_path(FILENAME); int write_fd = LIBC_NAMESPACE::open(TEST_FILE, O_WRONLY | O_CREAT, S_IRWXU); ASSERT_ERRNO_SUCCESS(); ASSERT_GT(write_fd, 0); - ASSERT_THAT(LIBC_NAMESPACE::close(write_fd), Succeeds(0)); - ASSERT_THAT(LIBC_NAMESPACE::unlink(TEST_FILE), Succeeds(0)); + + ASSERT_THAT(LIBC_NAMESPACE::close(write_fd), ESM::Succeeds(0)); + ASSERT_THAT(LIBC_NAMESPACE::unlink(TEST_FILE), ESM::Succeeds(0)); } -TEST(LlvmLibcUnlinkTest, UnlinkNonExistentFile) { - using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Fails; - ASSERT_THAT(LIBC_NAMESPACE::unlink("non-existent-file"), Fails(ENOENT)); +TEST_F(LlvmLibcUnlinkTest, UnlinkNonExistentFile) { + ASSERT_THAT(LIBC_NAMESPACE::unlink("non-existent-file"), ESM::Fails(ENOENT)); } diff --git a/utils/bazel/llvm-project-overlay/libc/test/UnitTest/BUILD.bazel b/utils/bazel/llvm-project-overlay/libc/test/UnitTest/BUILD.bazel index cec3fc16c62d1..27e468ddeb5f5 100644 --- a/utils/bazel/llvm-project-overlay/libc/test/UnitTest/BUILD.bazel +++ b/utils/bazel/llvm-project-overlay/libc/test/UnitTest/BUILD.bazel @@ -73,6 +73,17 @@ libc_test_library( ], ) +libc_test_library( + name = "errno_test_helpers", + hdrs = [ + "ErrnoCheckingTest.h", + ], + deps = [ + ":LibcUnitTest", + "//libc:errno", + ], +) + libc_test_library( name = "fp_test_helpers", srcs = [ diff --git a/utils/bazel/llvm-project-overlay/libc/test/src/unistd/BUILD.bazel b/utils/bazel/llvm-project-overlay/libc/test/src/unistd/BUILD.bazel index 66d8ddb0a67eb..0d6601633956f 100644 --- a/utils/bazel/llvm-project-overlay/libc/test/src/unistd/BUILD.bazel +++ b/utils/bazel/llvm-project-overlay/libc/test/src/unistd/BUILD.bazel @@ -248,6 +248,9 @@ libc_test( "//libc:close", "//libc:unlink", ], + deps = [ + "//libc/test/UnitTest:errno_test_helpers", + ], ) # libc_test( @@ -261,7 +264,6 @@ libc_test( # ], # ) - libc_test( name = "getppid_test", srcs = ["getppid_test.cpp"], From 88ffceb0de3021c9139b63bb1ac71504895ac048 Mon Sep 17 00:00:00 2001 From: Alexey Samsonov Date: Mon, 17 Mar 2025 18:17:04 -0700 Subject: [PATCH 2/4] update test --- libc/test/src/unistd/unlink_test.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/libc/test/src/unistd/unlink_test.cpp b/libc/test/src/unistd/unlink_test.cpp index 5921f4935001a..892779b30cb8e 100644 --- a/libc/test/src/unistd/unlink_test.cpp +++ b/libc/test/src/unistd/unlink_test.cpp @@ -16,19 +16,20 @@ #include using LlvmLibcUnlinkTest = LIBC_NAMESPACE::testing::ErrnoCheckingTest; -namespace ESM = LIBC_NAMESPACE::testing::ErrnoSetterMatcher; TEST_F(LlvmLibcUnlinkTest, CreateAndUnlink) { + using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Succeeds; constexpr const char *FILENAME = "unlink.test"; auto TEST_FILE = libc_make_test_file_path(FILENAME); int write_fd = LIBC_NAMESPACE::open(TEST_FILE, O_WRONLY | O_CREAT, S_IRWXU); ASSERT_ERRNO_SUCCESS(); ASSERT_GT(write_fd, 0); - ASSERT_THAT(LIBC_NAMESPACE::close(write_fd), ESM::Succeeds(0)); - ASSERT_THAT(LIBC_NAMESPACE::unlink(TEST_FILE), ESM::Succeeds(0)); + ASSERT_THAT(LIBC_NAMESPACE::close(write_fd), Succeeds(0)); + ASSERT_THAT(LIBC_NAMESPACE::unlink(TEST_FILE), Succeeds(0)); } TEST_F(LlvmLibcUnlinkTest, UnlinkNonExistentFile) { - ASSERT_THAT(LIBC_NAMESPACE::unlink("non-existent-file"), ESM::Fails(ENOENT)); + using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Fails; + ASSERT_THAT(LIBC_NAMESPACE::unlink("non-existent-file"), Fails(ENOENT)); } From d93f75a56059c0b58132bf53a5970c0914d587e1 Mon Sep 17 00:00:00 2001 From: Alexey Samsonov Date: Mon, 17 Mar 2025 18:23:44 -0700 Subject: [PATCH 3/4] clang-format --- libc/test/UnitTest/ErrnoCheckingTest.h | 1 - 1 file changed, 1 deletion(-) diff --git a/libc/test/UnitTest/ErrnoCheckingTest.h b/libc/test/UnitTest/ErrnoCheckingTest.h index c4583d09705fd..3d3b72f80544f 100644 --- a/libc/test/UnitTest/ErrnoCheckingTest.h +++ b/libc/test/UnitTest/ErrnoCheckingTest.h @@ -38,4 +38,3 @@ class ErrnoCheckingTest : public Test { } // namespace LIBC_NAMESPACE_DECL #endif // LLVM_LIBC_TEST_UNITTEST_ERRNOCHECKINGTEST_H - From a5058a10d826dbfd29629504435e3f78a099bbf7 Mon Sep 17 00:00:00 2001 From: Alexey Samsonov Date: Mon, 17 Mar 2025 20:11:10 -0700 Subject: [PATCH 4/4] restore CMake dependency --- libc/test/src/unistd/CMakeLists.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/libc/test/src/unistd/CMakeLists.txt b/libc/test/src/unistd/CMakeLists.txt index d0f1214b6609e..c31a03d4b27a5 100644 --- a/libc/test/src/unistd/CMakeLists.txt +++ b/libc/test/src/unistd/CMakeLists.txt @@ -353,6 +353,7 @@ add_libc_unittest( unlink_test.cpp DEPENDS libc.include.unistd + libc.src.errno.errno libc.src.fcntl.open libc.src.unistd.close libc.src.unistd.unlink