Skip to content
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

[C++] Test failures not propagated in Windows shared builds #21858

Closed
asfimport opened this issue May 23, 2019 · 9 comments
Closed

[C++] Test failures not propagated in Windows shared builds #21858

asfimport opened this issue May 23, 2019 · 9 comments
Assignees
Labels
Milestone

Comments

@asfimport
Copy link
Collaborator

See google/googletest#2261

Try e.g. this change:

diff --git a/cpp/src/arrow/buffer-test.cc b/cpp/src/arrow/buffer-test.cc
index 9b0530e5c..ce7628f55 100644
--- a/cpp/src/arrow/buffer-test.cc
+++ b/cpp/src/arrow/buffer-test.cc
@@ -35,6 +35,10 @@
 namespace arrow {

 TEST(TestAllocate, Bitmap) {
+  auto buf1 = Buffer::FromString("a");
+  auto buf2 = Buffer::FromString("b");
+  AssertBufferEqual(*buf1, *buf2);
+
   std::shared_ptr<Buffer> new_buffer;
   ARROW_EXPECT_OK(AllocateBitmap(default_memory_pool(), 100, &new_buffer));
   EXPECT_GE(new_buffer->size(), 13);

On a Windows shared library build, it outputs this:

[==========] Running 31 tests from 11 test cases.
[----------] Global test environment set-up.
[----------] 2 tests from TestAllocate
[ RUN      ] TestAllocate.Bitmap
..\src\arrow\testing\gtest_util.cc(120): error: Value of: buffer.Equals(expected
)
  Actual: false
Expected: true
[       OK ] TestAllocate.Bitmap (0 ms)
[ RUN      ] TestAllocate.EmptyBitmap
[       OK ] TestAllocate.EmptyBitmap (0 ms)
[----------] 2 tests from TestAllocate (0 ms total)

.... and the entire test file is marked passed.

Reporter: Antoine Pitrou / @pitrou
Assignee: Wes McKinney / @wesm

PRs and other links:

Note: This issue was originally created as ARROW-5403. Please see the migration documentation for further details.

@asfimport
Copy link
Collaborator Author

Uwe Korn / @xhochy:
Maybe we need to add ASSERT_NO_RAISES around AssertBufferEqual as we did in some other tests?

@asfimport
Copy link
Collaborator Author

Antoine Pitrou / @pitrou:
What is ASSERT_NO_RAISES? I can't find it anywhere.

@asfimport
Copy link
Collaborator Author

Uwe Korn / @xhochy:
Sorry, I meant ASSERT_NO_THROW

@asfimport
Copy link
Collaborator Author

Antoine Pitrou / @pitrou:
ASSERT_NO_THROW(...) doesn't change anything here. And besides, it would be a PITA to change all calls to test helpers.

I think that the solution may be to use a DLL version of gtest. However, massaging CMake to use the gtest DLL seems above my level of CMake proficiency.

@asfimport
Copy link
Collaborator Author

Antoine Pitrou / @pitrou:
By which I mean I'm trying the following changes:
https://gist.github.com/pitrou/e107d5ae292db5229a36026e113c867f

and get this cryptic error:

(arrow) c:\t\arrow\cpp\build-release>cmake --build . --config Release --target install
ninja: error: 'GTest::GTest-NOTFOUND', needed by 'release/arrow_testing.lib', missing and no known rule to make it

@asfimport
Copy link
Collaborator Author

Wes McKinney / @wesm:
I'm OK to take a look if you can push these changes to a branch

@asfimport
Copy link
Collaborator Author

Antoine Pitrou / @pitrou:
I'm working on something else right now. I think you can just take the gist above + the diff in the report above.

@asfimport
Copy link
Collaborator Author

Wes McKinney / @wesm:
OK, will do, thanks

@asfimport
Copy link
Collaborator Author

Wes McKinney / @wesm:
Issue resolved by pull request 4380
#4380

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants