Skip to content

[Support] Delete FormatVariadicTest Validate sub-test #106570

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 29, 2024

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
7 of 9 checks passed
@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