Skip to content

Conversation

@lntue
Copy link
Contributor

@lntue lntue commented Feb 21, 2025

No description provided.

…c.__support.File.file_test.__hermetic__

due to precommit bots's consistent failures.
@llvmbot
Copy link
Member

llvmbot commented Feb 21, 2025

@llvm/pr-subscribers-libc

Author: None (lntue)

Changes

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

1 Files Affected:

  • (modified) libc/test/src/__support/File/file_test.cpp (+40-37)
diff --git a/libc/test/src/__support/File/file_test.cpp b/libc/test/src/__support/File/file_test.cpp
index b3c9f2ba49bce..9c634ca07a5d0 100644
--- a/libc/test/src/__support/File/file_test.cpp
+++ b/libc/test/src/__support/File/file_test.cpp
@@ -113,43 +113,46 @@ StringFile *new_string_file(char *buffer, size_t buflen, int bufmode,
                              LIBC_NAMESPACE::File::mode_flags(mode));
 }
 
-TEST(LlvmLibcFileTest, WriteOnly) {
-  const char data[] = "hello, file";
-  constexpr size_t FILE_BUFFER_SIZE = sizeof(data) * 3 / 2;
-  char file_buffer[FILE_BUFFER_SIZE];
-  StringFile *f =
-      new_string_file(file_buffer, FILE_BUFFER_SIZE, _IOFBF, false, "w");
-
-  ASSERT_EQ(sizeof(data), f->write(data, sizeof(data)).value);
-  EXPECT_EQ(f->get_pos(), size_t(0)); // Data is buffered in the file stream
-  ASSERT_EQ(f->flush(), 0);
-  EXPECT_EQ(f->get_pos(), sizeof(data)); // Data should now be available
-  EXPECT_STREQ(f->get_str(), data);
-
-  f->reset();
-  ASSERT_EQ(f->get_pos(), size_t(0));
-  ASSERT_EQ(sizeof(data), f->write(data, sizeof(data)).value);
-  EXPECT_EQ(f->get_pos(), size_t(0)); // Data is buffered in the file stream
-  // The second write should trigger a buffer flush.
-  ASSERT_EQ(sizeof(data), f->write(data, sizeof(data)).value);
-  EXPECT_GE(f->get_pos(), size_t(0));
-  ASSERT_EQ(f->flush(), 0);
-  EXPECT_EQ(f->get_pos(), 2 * sizeof(data));
-  MemoryView src1("hello, file\0hello, file", sizeof(data) * 2),
-      dst1(f->get_str(), sizeof(data) * 2);
-  EXPECT_MEM_EQ(src1, dst1);
-
-  char read_data[sizeof(data)];
-  {
-    // This is not a readable file.
-    auto result = f->read(read_data, sizeof(data));
-    EXPECT_EQ(result.value, size_t(0));
-    EXPECT_TRUE(f->error());
-    EXPECT_TRUE(result.has_error());
-  }
-
-  ASSERT_EQ(f->close(), 0);
-}
+// TODO: Investigate the precommit bots' failures of this test and re-enable it.
+// https://github.com/llvm/llvm-project/issues/128185.
+//
+// TEST(LlvmLibcFileTest, WriteOnly) {
+//   const char data[] = "hello, file";
+//   constexpr size_t FILE_BUFFER_SIZE = sizeof(data) * 3 / 2;
+//   char file_buffer[FILE_BUFFER_SIZE];
+//   StringFile *f =
+//       new_string_file(file_buffer, FILE_BUFFER_SIZE, _IOFBF, false, "w");
+
+//   ASSERT_EQ(sizeof(data), f->write(data, sizeof(data)).value);
+//   EXPECT_EQ(f->get_pos(), size_t(0)); // Data is buffered in the file stream
+//   ASSERT_EQ(f->flush(), 0);
+//   EXPECT_EQ(f->get_pos(), sizeof(data)); // Data should now be available
+//   EXPECT_STREQ(f->get_str(), data);
+
+//   f->reset();
+//   ASSERT_EQ(f->get_pos(), size_t(0));
+//   ASSERT_EQ(sizeof(data), f->write(data, sizeof(data)).value);
+//   EXPECT_EQ(f->get_pos(), size_t(0)); // Data is buffered in the file stream
+//   // The second write should trigger a buffer flush.
+//   ASSERT_EQ(sizeof(data), f->write(data, sizeof(data)).value);
+//   EXPECT_GE(f->get_pos(), size_t(0));
+//   ASSERT_EQ(f->flush(), 0);
+//   EXPECT_EQ(f->get_pos(), 2 * sizeof(data));
+//   MemoryView src1("hello, file\0hello, file", sizeof(data) * 2),
+//       dst1(f->get_str(), sizeof(data) * 2);
+//   EXPECT_MEM_EQ(src1, dst1);
+
+//   char read_data[sizeof(data)];
+//   {
+//     // This is not a readable file.
+//     auto result = f->read(read_data, sizeof(data));
+//     EXPECT_EQ(result.value, size_t(0));
+//     EXPECT_TRUE(f->error());
+//     EXPECT_TRUE(result.has_error());
+//   }
+
+//   ASSERT_EQ(f->close(), 0);
+// }
 
 TEST(LlvmLibcFileTest, WriteLineBuffered) {
   const char data[] = "hello\n file";


ASSERT_EQ(f->close(), 0);
}
// TODO: Investigate the precommit bots' failures of this test and re-enable it.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use

#if 0
...
#endif

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Google Test supports disabling tests by prefixing their name with DISABLED_. This is better than commenting out the code or using #if 0, as disabled tests are still compiled (and thus won't rot). We should consider implementing the same feature in libc's test framework.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd probably want that to be target dependent as well, there's a handful of places where I need to disable a test only on NVPTX for example.

@lntue
Copy link
Contributor Author

lntue commented Feb 23, 2025

The root cause and fix for the issue is #128426

@lntue lntue closed this Feb 23, 2025
@lntue lntue deleted the file_test branch February 23, 2025 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants