Updates#744
Merged
Merged
Conversation
## Summary The harness was logging the entire raw `argv` of every invocation. The previous attempt filtered it through a hand-rolled parser; since we don't go through a real argv-parsing library, drop the log line entirely rather than ship a second parser to maintain. - **`src/main.ts`**: remove the `argv=` log line. - **`src/args.ts`**: remove the `extractKnownArgv` helper introduced in the prior commit. - **`__tests__/args.test.ts`**: remove the corresponding tests. ## Checklist - [x] A human-readable description of the changes was provided to include in CHANGELOG --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Peter Leonov <peter.leonov@clickhouse.com>
…23 newly-passing upstream tests (#734) - [x] Stand up the upstream test harness locally (ClickHouse server in Docker, sparse upstream checkout, build runner) - [x] Run a 100-test candidate batch (`head -100` of the not-yet-allowlisted 0_stateless tests) through the harness - [x] Identify the 12 passing tests in that batch - [x] Verify none of the 12 passers are bare prefixes of other 0_stateless tests - [x] Pick a focal failing test (`00030_alter_table`) and identify the root cause: the harness sends each `;`-separated statement as a separate, sessionless HTTP request, so `SET allow_deprecated_syntax_for_merge_tree = 1` is dropped before the next `CREATE TABLE ... ENGINE = MergeTree(d, k, 8192)` runs (server returns `BAD_ARGUMENTS`) - [x] **Commit #1**: add the 12 newly-passing tests to `upstream-allowlist.txt` - [x] **Commit #2**: harness fix (per-invocation `session_id`) + add 11 more tests it unblocks - [x] **Commit #3**: drop 23 allowlist entries that the `session_id` fix revealed as false-passes (parallel-replicas tests requiring named clusters, memory/parts/partition-limit tests, etc.) - [x] **Commit #4**: drop 2 more entries that were still failing after Commit #3: - `03594_alias_subcolumns` — flaky `result differs with reference` under upstream's random-settings rotation (the analyzer rewrites `not(equals(a.size0, 0))` ↔ `not(empty(aa))` and `sum(not(n.null))` ↔ `count(na)` depending on which `optimize_*` settings get rolled in) - `03928_materialized_cte_index` — bare-prefix entry that, per `tests/clickhouse-test` substring/prefix matching, pulls in the failing `03928_materialized_cte_index_2` sibling (needs `cluster_for_parallel_replicas = test_cluster_one_shard_three_replicas_localhost`, which the local docker-compose doesn't provide) - [ ] Verify CI is green on the next push --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: peter-leonov-ch <209667683+peter-leonov-ch@users.noreply.github.com>
## Summary
The `tests/clickhouse-test-runner` package was outside the root
`workspaces`, so its `@clickhouse/client: "*"` dependency resolved from
the npm registry. The new `upstream-sql-tests` workflow therefore
exercised the last published client instead of the PR checkout.
- **Workspaces**: add `./tests/clickhouse-test-runner` to root
`package.json` `workspaces`; delete the runner's now-obsolete
`package-lock.json`.
- **CI**: `.github/workflows/upstream-sql-tests.yml` runs `npm install
&& npm run build` from the repo root so all workspace packages
(including the runner) are linked and built before the harness runs.
- **Helper script**: drop the auto-build fallback in
`scripts/run-upstream-tests.sh`; callers (CI and contributors) are
expected to build from the repo root.
- **Docs / hints**: update `bin/clickhouse` error message,
`tests/clickhouse-test-runner/README.md`, and `AGENTS.md` to point at
the workspace-based install/build flow.
After this change, `require.resolve('@clickhouse/client', { paths:
['tests/clickhouse-test-runner'] })` resolves to
`packages/client-node/dist/index.js`.
## Checklist
- [x] A human-readable description of the changes was provided to
include in CHANGELOG
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: peter-leonov-ch <209667683+peter-leonov-ch@users.noreply.github.com>
Co-authored-by: Peter Leonov <peter.leonov@clickhouse.com>
## Summary `run-upstream-tests.sh` prepends `tests/clickhouse-test-runner/bin/` to `PATH`, but that directory only contained `clickhouse`. The upstream `tests/clickhouse-test` runner shells out to `clickhouse-client` to execute queries, so those invocations either bypassed the shim or failed when no real client was installed — contradicting the README's claim that the harness is a `clickhouse-client` replacement. - **`bin/clickhouse-client` symlink** → `clickhouse`, so both binary names resolve to the same wrapper. The existing shim already routes `extract-from-config` correctly and forwards everything else to `node dist/main.js`, so no script changes are needed. - **README** updated to document the symlink and why both names must be on `PATH` (`clickhouse extract-from-config` during setup, `clickhouse-client` for queries). ## Checklist - [x] A human-readable description of the changes was provided to include in CHANGELOG --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: peter-leonov-ch <209667683+peter-leonov-ch@users.noreply.github.com> Co-authored-by: Peter Leonov <peter.leonov@clickhouse.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
## Summary
Moves the examples-related CI jobs out of the catch-all `tests.yml` into
a dedicated `examples.yml` workflow so the examples surface has its own
check, status, and run history.
- **New `.github/workflows/examples.yml`** with two jobs:
- `code-quality` — matrix `[node, web]`, runs `npm run typecheck` and
`npm run lint` in `examples/${package}`.
- `run-examples` — matrix `{node, web} × {head, latest}` ClickHouse,
brings up Docker Compose and runs `npm run run-examples` (with
Playwright Chromium for `web`).
- Mirrors `tests.yml` for triggers (push to `main`, PR,
`workflow_dispatch`, daily cron), `permissions: {}`, concurrency group,
and OTEL env.
- **`.github/workflows/tests.yml`**: removed the `code-quality-examples`
and `run-examples` jobs and dropped them from the `success` aggregator's
`needs` list. No behavioral change to the moved jobs themselves — same
steps, same pinned action SHAs.
- **`.github/workflows/github-export-otel.yml`**: added `examples` to
the `workflow_run.workflows` allowlist so workflow telemetry continues
to be exported for runs of the new workflow alongside `tests`.
Note for reviewers: if `tests / success` was set as a required status
check on `main`, a corresponding `examples / success` check should be
added to branch protection.
## Checklist
- [x] A human-readable description of the changes was provided to
include in CHANGELOG
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: peter-leonov-ch <209667683+peter-leonov-ch@users.noreply.github.com>
Co-authored-by: Peter Leonov <peter.leonov@clickhouse.com>
## Summary A short description of the changes with a link to an open issue. ## Checklist Delete items not relevant to your PR: - [ ] Unit and integration tests covering the common scenarios were added - [ ] A human-readable description of the changes was provided to include in CHANGELOG - [ ] For significant changes, documentation in https://github.com/ClickHouse/clickhouse-docs was updated with further explanations or tutorials --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates CI organization and the upstream SQL test harness, moving example checks into their own workflow while making the ClickHouse test runner a root workspace that builds against local packages.
Changes:
- Adds a standalone examples workflow and removes example jobs from the main tests workflow.
- Converts the upstream SQL test runner to a root npm workspace and updates build docs/scripts.
- Adds per-invocation ClickHouse sessions for upstream test execution and adjusts the upstream allowlist.
Reviewed changes
Copilot reviewed 12 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
.github/instructions/review.instructions.md |
Adds repository review instructions. |
.github/workflows/examples.yml |
Adds standalone examples CI workflow. |
.github/workflows/github-export-otel.yml |
Adds telemetry export for the new examples workflow. |
.github/workflows/tests.yml |
Removes examples jobs from main tests workflow. |
.github/workflows/upstream-sql-tests.yml |
Builds the test runner from the repository root. |
AGENTS.md |
Documents root workspace behavior for the upstream SQL harness. |
package.json |
Adds the test runner as a root workspace. |
package-lock.json |
Updates lockfile for the new workspace layout. |
tests/clickhouse-test-runner/README.md |
Updates build and local development instructions. |
tests/clickhouse-test-runner/bin/clickhouse |
Updates missing-build guidance to point at the repo root. |
tests/clickhouse-test-runner/package-lock.json |
Removes the standalone test-runner lockfile. |
tests/clickhouse-test-runner/scripts/run-upstream-tests.sh |
Removes automatic local build from the runner script. |
tests/clickhouse-test-runner/src/backends/client.ts |
Adds a per-run session_id for multi-statement test execution. |
tests/clickhouse-test-runner/src/main.ts |
Stops logging raw argv. |
tests/clickhouse-test-runner/upstream-allowlist.txt |
Adds and removes upstream SQL test allowlist entries. |
Files not reviewed (1)
- tests/clickhouse-test-runner/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A short description of the changes with a link to an open issue.
Checklist
Delete items not relevant to your PR: