[Lldb-commits] [lldb] [lldb] Fixed PipeWindows bugs; added Pipe::WriteWithTimeout() (PR #101383)

[Pavel Labath via lldb-commits]

================
@@ -44,8 +42,70 @@ TEST_F(PipeTest, OpenAsReader) {
   size_t name_len = name.size();
   name += "foobar";
   llvm::StringRef name_ref(name.data(), name_len);
+  // Note OpenAsReader() do nothing on Windows, the pipe is already opened for
+  // read and write.
   ASSERT_THAT_ERROR(
       pipe.OpenAsReader(name_ref, /*child_process_inherit=*/false).ToError(),
       llvm::Succeeded());
+
+  ASSERT_TRUE(pipe.CanRead());
+}
+
+TEST_F(PipeTest, WriteWithTimeout) {
+  Pipe pipe;
+  ASSERT_THAT_ERROR(pipe.CreateNew(false).ToError(), llvm::Succeeded());
+  // Note write_chunk_size must be less than the pipe buffer.
+  // The pipe buffer is 1024 for PipeWindows and 4096 for PipePosix.
----------------
labath wrote:

The 4096 is actually system-dependent. It appears to be 512 on darwin, but I could imagine it can be bigger on some other system. Ideally the test would actually verify that the assumption is true. This is why I like the test plan I proposed in the other thread better. An assertion like this can be placed naturally within the code when it is structured as a sequence of steps, rather than a loop. E.g. something like this
1. write to the pipe until it is full -- this automatically ensures the above condition is met
2. attempt a write with a long(ish) timeout, check that it fails, and that it waits (a sufficient amount of time has passed)
3. attempt a write with a short timeout, check that it does not wait too long
4. drain the pipe
5. check that we got what we wrote
6. write to the pipe again and check that it succeeds

I hate to be a sore, but I think this provides better test coverage, and makes it clearer about what is being tested.

https://github.com/llvm/llvm-project/pull/101383