feat: Add IPC integration test executable #585
Conversation
bkietz
commented
Aug 13, 2024
•
- Adds a new executable for use with the archery integration tests
- When reading files, it ignores the Footer entirely
- A few private workarounds are added, most notably the stream writer tracks Blocks in order to populate the Footer
- Tested against patched archery apache/arrow@main...bkietz:arrow:nanoarrow-integration-tests
- issue to add nanoarrow to the tests [Integration]: Add nanoarrow to IPC integration tests arrow#43680
There was a problem hiding this comment.
I think this file would fit better as integration/ipc_integration.cc, next to the C data integration tests. You could separate them into a support lib/exe like Arrow C++, or you could build the main() function only if something is defined to ensure you can link to it from your tests.
I do think this would benefit from tests (e.g., to ensure error messages actually show up). I believe there are a few like that for the C Data integration functions (just enough to make sure everything is plugged in).
| return 1; | ||
| } | ||
|
|
||
| struct File { |
There was a problem hiding this comment.
No need to do it in this PR, but eventually we can consolidate/test the things that appear in both the C Data integration and IPC integration tests into nanoarrowarrow_testing
| if (argc == 1 || argv[1] == std::string{"-h"} || argv[1] == std::string{"--help"}) { | ||
| // skip printing usage if for example --gtest_list_tests is used; | ||
| // that command obviously doesn't need the extra noise | ||
| std::cerr << kUsage; | ||
| } | ||
| testing::InitGoogleTest(&argc, argv); | ||
| return RUN_ALL_TESTS(); |
There was a problem hiding this comment.
Is there any way to separate this in such a way that we're not using the same executable for both running tests and as the artifact needed for the integration test setup? Maybe a define to separate the two cases if a separate file is too hard? (No need to solve if this is time consuming!)
There was a problem hiding this comment.
I think this should wait for the MaterializedArrayStream follow up. When we move it to nanoarrow_testing.hpp from here and from c_data_integration.cc it'll be natural to consolidate MaterializedArrayStream's unit tests in nanoarrow_testing_test. Doing that here will bloat this PR excessively
f214889 to
0ddbb85
Compare
68e75e7 to
7d78c76
Compare
There was a problem hiding this comment.
Thanks!
We can tackle testing the failure modes and/or consolidating integration test logic in a separate PR. I have full faith in your implementation but for when this is modified by a future contributor we do need a basic check to make sure that if there is a validation failure that it is actually reported (since that type of failure would be silently unnoticed in the normal integration test run).
…3715) ### Rationale for this change Nanoarrow can now read and write IPC files as of apache/arrow-nanoarrow#585 so it should no longer be skipped as a producer/consumer ### What changes are included in this PR? Nanoarrow's tester is updated to point to the new integration executable and to report nanoarrow as a consumer/producer of IPC files. Notably the `null_trivial` case is skipped even though nanoarrow nominally supports it since it represents a corner case in which nanoarrow's flatbuffers library will not accept some vectors produced by other flatbuffers libraries dvidelabs/flatcc#287 ### Are these changes tested? Yes ### Are there any user-facing changes? No * GitHub Issue: #43680 Lead-authored-by: Benjamin Kietzman <bengilgit@gmail.com> Co-authored-by: Antoine Pitrou <pitrou@free.fr> Signed-off-by: Benjamin Kietzman <bengilgit@gmail.com>