Skip to content

chore(velox-connector): Remove the C++ tests that were never built. - #64

Open
jackluo923 wants to merge 23 commits into
stack/integration-tests/11-cpp-unit-testsfrom
stack/integration-tests/10-remove-cpp-tests
Open

jackluo923 wants to merge 23 commits into
stack/integration-tests/11-cpp-unit-testsfrom
stack/integration-tests/10-remove-cpp-tests

Conversation

@jackluo923

@jackluo923 jackluo923 commented Aug 4, 2026 •

Copy link
Copy Markdown
Member

Description

Removes ClpConnectorTest.cpp, ClpConfigTest.cpp, and the 13 example archives that they read.

No CMakeLists.txt references them, so they have never been built. Wiring them up is impractical: the plugin leaves Velox symbols undefined so that the worker process resolves them at load time, which leaves a test binary with nothing to link against unless Velox is compiled into the build-env image.

Their cases are covered by the integration tests, and by the velox-connector/tests/ unit tests that this branch already carries.

Breaking changes

None.

Validation performed

git grep finds no remaining reference to the deleted files or their examples/ directory.

task velox-connector:test: 11 passed. task integration-tests:run: 24 passed, 2 xfailed.

Checklist

  • The PR satisfies the contribution guidelines.
  • Necessary docs have been updated, OR no docs need to be updated.
  • The description has been filled out.
  • The breaking changes section has been filled out.
  • The validation performed section has been filled out.
  • The PR title:
    • follows the Conventional Commits specification.
    • uses one of the commit types from here.
    • is in imperative form (e.g., "Add new node types.").
    • references the GitHub issues that the PR resolves (if any) using the (fixes #N) or
      (resolves #N) syntax in the title.

@coderabbitai

coderabbitai Bot commented Aug 4, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 479df8d3-a18f-4100-805b-f6ebcd193913

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@jackluo923
jackluo923 force-pushed the stack/integration-tests/10-remove-cpp-tests branch 2 times, most recently from cd9ea8f to 0b74ad3 Compare August 4, 2026 02:29
@jackluo923
jackluo923 force-pushed the stack/integration-tests/10-remove-cpp-tests branch from 0b74ad3 to 4f815e1 Compare August 4, 2026 02:34
@jackluo923
jackluo923 force-pushed the stack/integration-tests/10-remove-cpp-tests branch 2 times, most recently from 0324d12 to 5ca430e Compare August 4, 2026 03:19
@jackluo923
jackluo923 force-pushed the stack/integration-tests/10-remove-cpp-tests branch from 5ca430e to 425b1d1 Compare August 4, 2026 03:31
@jackluo923
jackluo923 force-pushed the stack/integration-tests/10-remove-cpp-tests branch 2 times, most recently from 62c027f to f2b9d50 Compare August 4, 2026 04:16
@jackluo923
jackluo923 force-pushed the stack/integration-tests/10-remove-cpp-tests branch 2 times, most recently from 006c337 to fb97f1c Compare August 4, 2026 04:56
@jackluo923
jackluo923 force-pushed the stack/integration-tests/10-remove-cpp-tests branch from fb97f1c to f53c675 Compare August 4, 2026 05:08
@jackluo923
jackluo923 force-pushed the stack/integration-tests/10-remove-cpp-tests branch 2 times, most recently from 59e63e4 to 1b836ca Compare August 4, 2026 08:04
@jackluo923
jackluo923 changed the base branch from stack/integration-tests/09-test-ci to stack/integration-tests/09b-s3-storage August 4, 2026 08:07
@jackluo923
jackluo923 force-pushed the stack/integration-tests/10-remove-cpp-tests branch from 1b836ca to 23a9358 Compare August 4, 2026 08:44
@jackluo923
jackluo923 changed the base branch from stack/integration-tests/09b-s3-storage to stack/integration-tests/11-cpp-unit-tests August 4, 2026 08:45
@jackluo923
jackluo923 force-pushed the stack/integration-tests/10-remove-cpp-tests branch from 23a9358 to 00d735e Compare August 4, 2026 08:46
@jackluo923
jackluo923 force-pushed the stack/integration-tests/10-remove-cpp-tests branch 2 times, most recently from 7357854 to ce34c77 Compare August 5, 2026 19:51
Removes `ClpConnectorTest.cpp`, `ClpConfigTest.cpp`, and the 13 example archives they read.

No `CMakeLists.txt` has ever referenced them. They arrived with the initial plugin commit and were never compiled or run, because the plugin is a shared library that deliberately leaves Velox symbols undefined for the worker to resolve at load time: a test binary has nothing to link against, and building one would mean compiling Velox into the build-env image.

Their cases are covered now. The read-path and pushdown ones run as SQL against a live worker in the integration tests, and the S3 auth and URL ones, which no SQL can reach, run as unit tests alongside the connector's Velox-free headers.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@jackluo923
jackluo923 force-pushed the stack/integration-tests/10-remove-cpp-tests branch from ce34c77 to 8c3c860 Compare August 5, 2026 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants