Skip to content

Conversation

@jurahul
Copy link
Contributor

@jurahul jurahul commented Aug 29, 2024

  • The subtest, if enabled correctly, will fail with assert in Debug builds and validation is disabled in Release builds.
  • Hence deleting the test to fix test failures in CI.

- The subtest will fail assert in Debug builds and validation is
  disabled in Release builds, so effectively it won't be possible
  to test validation in a unit test.
@llvmbot
Copy link
Member

llvmbot commented Aug 29, 2024

@llvm/pr-subscribers-llvm-support

Author: Rahul Joshi (jurahul)

Changes
  • The subtest will fail assert in Debug builds and validation is disabled in Release builds, so effectively it won't be possible to test validation in a unit test.

Full diff: https://github.com/llvm/llvm-project/pull/106570.diff

1 Files Affected:

  • (modified) llvm/unittests/Support/FormatVariadicTest.cpp (-10)
diff --git a/llvm/unittests/Support/FormatVariadicTest.cpp b/llvm/unittests/Support/FormatVariadicTest.cpp
index 4c648d87fc2de7..6ee0d924867419 100644
--- a/llvm/unittests/Support/FormatVariadicTest.cpp
+++ b/llvm/unittests/Support/FormatVariadicTest.cpp
@@ -710,16 +710,6 @@ TEST(FormatVariadicTest, FormatFilterRange) {
   EXPECT_EQ("1, 2, 3", formatv("{0}", Range).str());
 }
 
-#ifdef NDEBUG // Disable the test in debug builds where it will assert.
-TEST(FormatVariadicTest, Validate) {
-  std::string Str = formatv("{0}", 1, 2).str();
-  EXPECT_THAT(Str, HasSubstr("Unexpected number of arguments"));
-
-  Str = formatv("{0} {2}", 1, 2, 3).str();
-  EXPECT_THAT(Str, HasSubstr("eplacement indices have holes"));
-}
-#endif // NDEBUG
-
 namespace {
 
 enum class Base { First };

@jurahul
Copy link
Contributor Author

jurahul commented Aug 29, 2024

Looks like I broke the builds with my earlier commit as this subtest is now failing. I am disabling it for now, and will need to rewrite it to use ASSERT_DEATH etc, which I'll do in a follow on commit, but want to fix the build first.

@jurahul jurahul requested a review from jpienaar August 29, 2024 15:30
@jurahul
Copy link
Contributor Author

jurahul commented Aug 29, 2024

Looks like I broke the builds with my earlier commit

The guard condition is not correct. But fixing that is not sufficient as the test will assert in debug mode. So effectively we need to delete the test.

pre-commit CI did not catch this issue since it runs with assertions enabled (NDEBUG undefined), so #ifdef NDEBUG was false and the test was disabed.

@jurahul jurahul merged commit 5048fab into llvm:main Aug 29, 2024
@jurahul jurahul deleted the fix_formatvariadic_test branch August 29, 2024 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants