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

[CI][Docs][MATLAB] Remove GoogleTest support from the CMake build system for the MATLAB interface #37532

Closed
kou opened this issue Sep 4, 2023 · 1 comment · Fixed by #37784

Comments

@kou
Copy link
Member

kou commented Sep 4, 2023

Describe the enhancement requested

Now, our build system has GoogleTest support but it's not used. There is only a placeholder test that tests nothing.
We had one test for mex_util but it removed by GH-37204.
We use the MATLAB's testing framework for all tests.

The GoogleTest support needs non-trivial CMake codes. If we can drop the GoogleTest support, we can reduce maintenance cost for it and reduce build time of the MATLAB bindings.

But we have the following use-case of the GoogleTest support:

#37483 (comment)

I think we want to keep the GoogleTest support for now because we do plan on adding C++ unit tests. There are a few error-conditions that cannot be triggered from MATLAB for which we want to add C++ unit tests.

Can we use another approach for the use-case?

For example, can we write a C++ method that causes an error-condition and register the method to MATLAB?

diff --git a/matlab/src/cpp/arrow/matlab/array/proxy/array.cc b/matlab/src/cpp/arrow/matlab/array/proxy/array.cc
index 9dc14c341..cedb58793 100644
--- a/matlab/src/cpp/arrow/matlab/array/proxy/array.cc
+++ b/matlab/src/cpp/arrow/matlab/array/proxy/array.cc
@@ -37,6 +37,9 @@ namespace arrow::matlab::array::proxy {
         REGISTER_METHOD(Array, type);
         REGISTER_METHOD(Array, isEqual);
 
+#ifdef ARROW_MATLAB_TEST
+        REGISTER_METHOD(Array, errorA);
+#endif
     }
 
     std::shared_ptr<arrow::Array> Array::unwrap() {
@@ -115,4 +118,11 @@ namespace arrow::matlab::array::proxy {
         mda::ArrayFactory factory;
         context.outputs[0] = factory.createScalar(is_equal);
     }
+
+#ifdef ARROW_MATLAB_TEST
+    void Array::errorA(libmexclass::proxy::method::Context& context) {
+        auto errorCondition = arrow::Status::Invalid("error A!");
+        MATLAB_ERROR_IF_NOT_OK_WITH_CONTEXT(errorCondition, context, ...ERROR_ID);
+    }
+#endif
 }

If we can call the method from MATLAB, can we assert it by the MATLAB testing framework?

FYI: We removed GoogleTest dependency from PyArrow by GH-14117/GH-32328 because maintaining GoogleTest support in python/ was high cost for us.
only when

Component(s)

MATLAB

@kevingurney kevingurney self-assigned this Sep 18, 2023
@kevingurney kevingurney changed the title [MATLAB] Reconsider GoogleTest needs [CI][Docs][MATLAB] Remove GoogleTest support from the CMake build system for the MATLAB interface Sep 18, 2023
@kevingurney
Copy link
Member

  1. Re-worded title of this issue.
  2. Added labels Component: Documentation and Component: Continuous Integration since this impacts the documentation and MATLAB CI workflows.

kevingurney added a commit that referenced this issue Sep 19, 2023
…ke build system for the MATLAB interface (#37784)

### Rationale for this change

This pull request removes `GoogleTest` support from the CMake build system for the MATLAB interface.

1. `GoogleTest` support adds a lot of additional complexity to the CMake build system for the MATLAB interface, and we currently don't have any standalone C++ tests for the MATLAB interface code.
2. In order to use `GoogleTest` in the MATLAB CI workflows, we are currently relying on building the tests for the Arrow C++ libraries in order to "re-use" the `GoogleTest binaries. This adds additional overhead to the MATLAB CI workflows.
3. If we want to test some internal C++ code for the MATLAB interface in the future, we can instead use a MEX function to call the code from a MATLAB test as suggested by @ kou in #37532 (comment).
4. There is [precedent for testing internal C++ code without GoogleTest for the Python bindings](#14117).
5. On a somewhat related note - removing `GoogleTest` support will help unblock #37773 as discussed in #37773 (comment).

### What changes are included in this PR?

1. Removed the `MATLAB_BUILD_TESTS` flag from the CMake build system for the MATLAB interface since there are no longer any C++ tests for the MATLAB interface to build.
2. Updated the `matlab_build.sh` CI workflow script to avoid building the tests for the Arrow C++ libraries and to no longer call `ctest`.
3. Updated the `README.md` for the MATLAB interface to no longer mention building or running C++ tests.
4. Updated the design document for the MATLAB Interface to no longer mention `GoogleTest` since we may end up testing internal C++ code using MEX function calls from MATLAB instead.
5. Removed placeholder C++ test (i.e. `placeholder_test.cc`).

### Are these changes tested?

Yes.

The MATLAB CI workflow is passing on all platforms.

### Are there any user-facing changes?

Yes.

There are no longer any C++ tests for the MATLAB interface. The `MATLAB_BUILD_TESTS` flag has been removed from the CMake build system to reflect this change. If a user supplies a value for `MATLAB_BUILD_TESTS` when building the MATLAB interface, the flag will be ignored by CMake.

### Future Directions

1. Add more developer-focused documentation on how to test C++ code via MEX function calls from MATLAB.

### Notes

1. In the future, we can consider testing internal C++ code using MEX function calls from MATLAB tests as suggested by @ kou in #37532 (comment). Currently, we don't have any C++ tests that need to be adapted to use this approach.
2. Thank you @ sgilmore10 for your help with this pull request!
* Closes: #37532

Lead-authored-by: Kevin Gurney <kgurney@mathworks.com>
Co-authored-by: Sarah Gilmore <sgilmore@mathworks.com>
Signed-off-by: Kevin Gurney <kgurney@mathworks.com>
@kevingurney kevingurney added this to the 14.0.0 milestone Sep 19, 2023
loicalleyne pushed a commit to loicalleyne/arrow that referenced this issue Nov 13, 2023
…he CMake build system for the MATLAB interface (apache#37784)

### Rationale for this change

This pull request removes `GoogleTest` support from the CMake build system for the MATLAB interface.

1. `GoogleTest` support adds a lot of additional complexity to the CMake build system for the MATLAB interface, and we currently don't have any standalone C++ tests for the MATLAB interface code.
2. In order to use `GoogleTest` in the MATLAB CI workflows, we are currently relying on building the tests for the Arrow C++ libraries in order to "re-use" the `GoogleTest binaries. This adds additional overhead to the MATLAB CI workflows.
3. If we want to test some internal C++ code for the MATLAB interface in the future, we can instead use a MEX function to call the code from a MATLAB test as suggested by @ kou in apache#37532 (comment).
4. There is [precedent for testing internal C++ code without GoogleTest for the Python bindings](apache#14117).
5. On a somewhat related note - removing `GoogleTest` support will help unblock apache#37773 as discussed in apache#37773 (comment).

### What changes are included in this PR?

1. Removed the `MATLAB_BUILD_TESTS` flag from the CMake build system for the MATLAB interface since there are no longer any C++ tests for the MATLAB interface to build.
2. Updated the `matlab_build.sh` CI workflow script to avoid building the tests for the Arrow C++ libraries and to no longer call `ctest`.
3. Updated the `README.md` for the MATLAB interface to no longer mention building or running C++ tests.
4. Updated the design document for the MATLAB Interface to no longer mention `GoogleTest` since we may end up testing internal C++ code using MEX function calls from MATLAB instead.
5. Removed placeholder C++ test (i.e. `placeholder_test.cc`).

### Are these changes tested?

Yes.

The MATLAB CI workflow is passing on all platforms.

### Are there any user-facing changes?

Yes.

There are no longer any C++ tests for the MATLAB interface. The `MATLAB_BUILD_TESTS` flag has been removed from the CMake build system to reflect this change. If a user supplies a value for `MATLAB_BUILD_TESTS` when building the MATLAB interface, the flag will be ignored by CMake.

### Future Directions

1. Add more developer-focused documentation on how to test C++ code via MEX function calls from MATLAB.

### Notes

1. In the future, we can consider testing internal C++ code using MEX function calls from MATLAB tests as suggested by @ kou in apache#37532 (comment). Currently, we don't have any C++ tests that need to be adapted to use this approach.
2. Thank you @ sgilmore10 for your help with this pull request!
* Closes: apache#37532

Lead-authored-by: Kevin Gurney <kgurney@mathworks.com>
Co-authored-by: Sarah Gilmore <sgilmore@mathworks.com>
Signed-off-by: Kevin Gurney <kgurney@mathworks.com>
dgreiss pushed a commit to dgreiss/arrow that referenced this issue Feb 19, 2024
…he CMake build system for the MATLAB interface (apache#37784)

### Rationale for this change

This pull request removes `GoogleTest` support from the CMake build system for the MATLAB interface.

1. `GoogleTest` support adds a lot of additional complexity to the CMake build system for the MATLAB interface, and we currently don't have any standalone C++ tests for the MATLAB interface code.
2. In order to use `GoogleTest` in the MATLAB CI workflows, we are currently relying on building the tests for the Arrow C++ libraries in order to "re-use" the `GoogleTest binaries. This adds additional overhead to the MATLAB CI workflows.
3. If we want to test some internal C++ code for the MATLAB interface in the future, we can instead use a MEX function to call the code from a MATLAB test as suggested by @ kou in apache#37532 (comment).
4. There is [precedent for testing internal C++ code without GoogleTest for the Python bindings](apache#14117).
5. On a somewhat related note - removing `GoogleTest` support will help unblock apache#37773 as discussed in apache#37773 (comment).

### What changes are included in this PR?

1. Removed the `MATLAB_BUILD_TESTS` flag from the CMake build system for the MATLAB interface since there are no longer any C++ tests for the MATLAB interface to build.
2. Updated the `matlab_build.sh` CI workflow script to avoid building the tests for the Arrow C++ libraries and to no longer call `ctest`.
3. Updated the `README.md` for the MATLAB interface to no longer mention building or running C++ tests.
4. Updated the design document for the MATLAB Interface to no longer mention `GoogleTest` since we may end up testing internal C++ code using MEX function calls from MATLAB instead.
5. Removed placeholder C++ test (i.e. `placeholder_test.cc`).

### Are these changes tested?

Yes.

The MATLAB CI workflow is passing on all platforms.

### Are there any user-facing changes?

Yes.

There are no longer any C++ tests for the MATLAB interface. The `MATLAB_BUILD_TESTS` flag has been removed from the CMake build system to reflect this change. If a user supplies a value for `MATLAB_BUILD_TESTS` when building the MATLAB interface, the flag will be ignored by CMake.

### Future Directions

1. Add more developer-focused documentation on how to test C++ code via MEX function calls from MATLAB.

### Notes

1. In the future, we can consider testing internal C++ code using MEX function calls from MATLAB tests as suggested by @ kou in apache#37532 (comment). Currently, we don't have any C++ tests that need to be adapted to use this approach.
2. Thank you @ sgilmore10 for your help with this pull request!
* Closes: apache#37532

Lead-authored-by: Kevin Gurney <kgurney@mathworks.com>
Co-authored-by: Sarah Gilmore <sgilmore@mathworks.com>
Signed-off-by: Kevin Gurney <kgurney@mathworks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment