-
Notifications
You must be signed in to change notification settings - Fork 4k
GH-47434: [C++] Fix issue preventing running of tests on Windows #47455
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
Conversation
|
|
|
OK looks like tests are running now. There's an expected test failure causing CI to fail, for which an issue has been filed in #47442 For now I'll macro out the part that doesn't work on Windows to get CI green |
|
Hmm the orc failure seems like it might be something with the Orc library itself? That error message seems to point back here, although I'm not sure why it would that condition would be true on Windows: Maybe something @wgtmac has an idea on? |
|
The substrait appears to not like the way we are setting the path to the parquet test data in the cpp test script: Line 33 in a4cde2c
Open to ideas on how to best make that cross-platform |
|
The failed test only runs for ORC version below 2.0 The Windows CI is running with ORC 1.9.0 which is way too old: Can we use a new version of ORC? |
|
Hmm interesting. It looks like that is being installed via conda-forge, but I don't see it pinned to that particular version. Not sure why it wouldn't pick up something newer |
|
AFAICT there is a version dependency conflict between grpc and libprotobuf that is pinning orc to 1.9.0. Here's the dependency solve: Details├─ google-cloud-cpp >=1.34.0 * is installable with the potential options Perhaps for now we should just skip that test on Windows? |
|
The purpose of that test is for Windows where TZDB might be missing. Since it is for Apache ORC pre-2.0 versions, we can disable it now. |
raulcd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good finding! Apart from the minor lint issue, I can see this pattern also on gandiva tests, should we also update to ARROW_GTEST_GMOCK_MAIN?
arrow/cpp/src/gandiva/CMakeLists.txt
Lines 198 to 199 in 743d0af
| list(APPEND GANDIVA_STATIC_TEST_LINK_LIBS ${ARROW_GTEST_GMOCK} ${ARROW_GTEST_GTEST_MAIN}) | |
| list(APPEND GANDIVA_SHARED_TEST_LINK_LIBS ${ARROW_GTEST_GMOCK} ${ARROW_GTEST_GTEST_MAIN}) |
|
Nice catch @raulcd - that makes sense to update as well |
|
Thanks for the approval @raulcd . Is this OK for me to merge now? I see the bot added that label, but wanted to double check before running the script. I'm not sure if the logic for that tag is reflective of what the team always wants |
|
Unless you want someone else to take a look, this seems ok to be merged, it has two approvals already! |
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 3105d9c. There weren't enough matching historic benchmark results to make a call on whether there were regressions. The full Conbench report has more details. |
apache#47455) ### Rationale for this change It was recently discovered that the majority of tests in the CMake configuration on Windows are not actually running. This solves that issue so the tests are discovered and run. ### What changes are included in this PR? Usage of the gtest_main dependency has been replaced with gmock_main. ### Are these changes tested? Yes ### Are there any user-facing changes? No * GitHub Issue: apache#47434 Authored-by: Will Ayd <[email protected]> Signed-off-by: Will Ayd <[email protected]>
Rationale for this change
It was recently discovered that the majority of tests in the CMake configuration on Windows are not actually running. This solves that issue so the tests are discovered and run.
What changes are included in this PR?
Usage of the gtest_main dependency has been replaced with gmock_main.
Are these changes tested?
Yes
Are there any user-facing changes?
No