test: Make Arrow C++ dependency optional #677

WillAyd · 2024-11-09T18:27:46Z

No description provided.

codecov-commenter · 2024-11-09T18:46:01Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.63%. Comparing base (44e8eb9) to head (e7959e0).
Report is 29 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #677      +/-   ##
==========================================
+ Coverage   86.29%   87.63%   +1.34%     
==========================================
  Files          94      101       +7     
  Lines       13604    14537     +933     
==========================================
+ Hits        11739    12739    +1000     
+ Misses       1865     1798      -67

Flag	Coverage Δ
	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

paleolimbot

I know you're still working on this, but just two high level comments to think about:

We probably want a CMake option of -DNANOARROW_TESTS_WITH_ARROW (where if it's ON, configuration will fail if Arrow is not found, and if it's OFF, tests are never built with arrow). Having it be automatic makes it relatively easy to accidentally think you are testing with Arrow C++ when CMake actually failed to locate it.
There are a few tests where the entire test is useless, but many of them only have Arrow C++ as a small component (notably: the schema init ones). It doesn't have to be perfect at the end of this PR, but before the next release we will ideally have full (or at least not reduced) test coverage without any of the Arrow C++ integration tests running.
An alternative to compiling tests with a define is to move the (portions of) tests that need Arrow C++ to separate files (Arrow C++-specific integration tests, perhaps under integration/).

paleolimbot · 2024-11-09T21:06:29Z

CMakeLists.txt

+    # TODO: does this propoagate to projects using as a subproject?
+    add_compile_definitions("-DNANOARROW_ARROW_FOUND")


I think it does. You probably want target_compile_definitions(xxxx_test PRIVATE -DNANOARROW_ARROW_FOUND) for every test because the tests are what actually need this define set. You could do something fancy like we do with the nanoarrow_coverage_config (define an interface target that the tests "link to"), or bite the bullet and use a foreach() loop to link tests (sort of like you do in the meson config), which we probably should have done a while ago anyway.

WillAyd · 2024-11-12T20:46:57Z

CMakeLists.txt

@@ -517,6 +520,27 @@ if(NANOARROW_BUILD_TESTS)
                        gmock_main
                        nanoarrow_coverage_config)

+  list(APPEND


Just kept the loop scoped to adding the define for now, although there is (as you've already mentioned) potential to use this for other parts of the config

WillAyd · 2024-11-12T20:47:42Z

src/nanoarrow/common/array_test.cc

@@ -1719,7 +1812,8 @@ TEST(ArrayTest, ArrayTestAppendToRunEndEncodedArray) {
  EXPECT_EQ(ArrowArrayFinishBuilding(&array, NANOARROW_VALIDATION_LEVEL_FULL, &error),
            NANOARROW_OK);

-#if !defined(ARROW_VERSION_MAJOR) || ARROW_VERSION_MAJOR < 12
+#if !defined(NANOARROW_BUILD_TESTS_WITH_ARROW) || !defined(ARROW_VERSION_MAJOR) || \


Do we want to add this here or leave as is?

Either is OK by me! I think it makes sense that if the version isn't defined that we didn't build with arrow C++ (but also it might make searching for all the Arrow C++ conditional test blocks easier to include the extra check).

paleolimbot · 2024-11-13T03:32:18Z

CMakeLists.txt

-  find_package(Arrow REQUIRED)
-  message(STATUS "Arrow version: ${ARROW_VERSION}")
-  message(STATUS "Arrow SO version: ${ARROW_FULL_SO_VERSION}")
+  find_package(Arrow)


Suggested change

find_package(Arrow)

if(NANOARROW_BUILD_TESTS_WITH_ARROW)

find_package(Arrow REQUIRED)

endif()

...maybe?

paleolimbot · 2024-11-13T03:34:34Z

CMakePresets.json

@@ -21,6 +21,7 @@
            "cacheVariables": {
                "CMAKE_BUILD_TYPE": "Debug",
                "NANOARROW_BUILD_TESTS": "ON"
+                "NANOARROW_BUILD_TESTS_WITH_ARROW": "ON"


I think we should probably leave this out of the defaults (I would keep it in my own CMakeUserPresets, but most people wouldn't need this when making their first contribution).

paleolimbot · 2024-11-13T18:09:01Z

dev/release/verify-release-candidate.sh

@@ -243,12 +243,14 @@ test_cmake_project() {

 test_c() {
  show_header "Build and test C library"
-  test_cmake_project build . -DNANOARROW_BUILD_TESTS=ON -DNANOARROW_IPC=ON
+  test_cmake_project build . -DNANOARROW_BUILD_TESTS=ON -DNANOARROW_IPC=ON \
+                     -DNANOARROW_BUILD_TESTS_WITH_ARROW=ON


For verification it would be useful to default this to OFF (because it significantly improves the instructions required for verifying). It can always be turned on (and perhaps we should turn it on in .github/workflows/verify.yaml) via the extra cmake args.

paleolimbot · 2024-11-13T18:11:33Z

src/nanoarrow/common/array_test.cc

@@ -1719,7 +1812,8 @@ TEST(ArrayTest, ArrayTestAppendToRunEndEncodedArray) {
  EXPECT_EQ(ArrowArrayFinishBuilding(&array, NANOARROW_VALIDATION_LEVEL_FULL, &error),
            NANOARROW_OK);

-#if !defined(ARROW_VERSION_MAJOR) || ARROW_VERSION_MAJOR < 12
+#if !defined(NANOARROW_BUILD_TESTS_WITH_ARROW) || !defined(ARROW_VERSION_MAJOR) || \


Either is OK by me! I think it makes sense that if the version isn't defined that we didn't build with arrow C++ (but also it might make searching for all the Arrow C++ conditional test blocks easier to include the extra check).

WillAyd force-pushed the optional-arrow branch from ad45d3e to bc80f47 Compare November 9, 2024 18:40

paleolimbot reviewed Nov 9, 2024

View reviewed changes

WillAyd force-pushed the optional-arrow branch from bc80f47 to 3ffc2f7 Compare November 12, 2024 18:38

WillAyd commented Nov 12, 2024

View reviewed changes

WillAyd added 7 commits November 13, 2024 12:09

test: Make Arrow C++ dependency optional

99e72e5

Add option for testing without Arrow

9198c20

Meson configuration cleanup

fa52487

Narrower ifdefined scopes

d9f7b36

Updated CMake Configuration

ff4b4b2

fix test leak

d465717

fix rebase issue

7db51a6

WillAyd force-pushed the optional-arrow branch from e7959e0 to 7db51a6 Compare November 13, 2024 17:14

paleolimbot reviewed Nov 13, 2024

View reviewed changes

WillAyd force-pushed the optional-arrow branch 2 times, most recently from f0b8ca9 to 01ce608 Compare November 14, 2024 19:26

Feedback

76af997

WillAyd force-pushed the optional-arrow branch from 01ce608 to 76af997 Compare November 14, 2024 19:27

WillAyd marked this pull request as ready for review November 14, 2024 19:43

Updated README

6d349c7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: Make Arrow C++ dependency optional #677

test: Make Arrow C++ dependency optional #677

WillAyd commented Nov 9, 2024

codecov-commenter commented Nov 9, 2024 •

edited

Loading

paleolimbot left a comment

paleolimbot Nov 9, 2024

WillAyd Nov 12, 2024

WillAyd Nov 12, 2024

paleolimbot Nov 13, 2024

paleolimbot Nov 13, 2024

paleolimbot Nov 13, 2024

paleolimbot Nov 13, 2024

paleolimbot Nov 13, 2024

		# TODO: does this propoagate to projects using as a subproject?
		add_compile_definitions("-DNANOARROW_ARROW_FOUND")

test: Make Arrow C++ dependency optional #677

Are you sure you want to change the base?

test: Make Arrow C++ dependency optional #677

Conversation

WillAyd commented Nov 9, 2024

codecov-commenter commented Nov 9, 2024 • edited Loading

Codecov Report

paleolimbot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 9, 2024 •

edited

Loading