Skip to content

Commit c4e2dc0

Browse files
docs(bots): teach backend selection (Thrift/SEA/kernel) + realkernel tiers
Follow-up to the #870 review thread on --all-extras: the bot needs to know, per issue, WHICH backend the bug is on and reproduce on that one — a Thrift bug won't reproduce on a kernel connection, and a broad unit run with the real kernel wheel present false-reds unless realkernel is deselected. That knowledge was tribal; write it down. - CONTRIBUTING.md: add a "Backends and test tiers" section — the three backends (Thrift default / SEA `use_sea=True` / kernel `use_kernel=True`), where each backend's tests live, that kernel is an opt-in extra, and the rule that `realkernel` tests run in their own invocation (`-m "not realkernel"` for broad runs), matching how CI (code-coverage.yml / code-quality-checks.yml) splits them. - engineer/system.md: add step 0 — pick the backend the bug is on and reproduce there; point to the CONTRIBUTING matrix. - engineer-followup/system.md: correct the stale "do NOT run tests/e2e" line (the followup job now has live creds via #870) and point at the same backend matrix. Keeps --all-extras (both backends supported); the residual "prompt-discipline only" risk the reviewer flagged is now backed by a documented, human-shared convention plus explicit bot rules. Signed-off-by: eric-wang-1990 <e.wang@databricks.com> Co-authored-by: Isaac
1 parent 7c0ca1b commit c4e2dc0

3 files changed

Lines changed: 51 additions & 7 deletions

File tree

.bot/prompts/engineer-followup/system.md

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,13 @@ Your job:
2727
4. End with a short summary of what changed.
2828

2929
Repo facts you need:
30-
- `poetry`-managed, Python 3.8+; `poetry install` has run on the runner, so
31-
`poetry run python -m pytest tests/unit` runs the fully-mocked unit suite
32-
with no warehouse. Do NOT run or add `tests/e2e` (needs live credentials).
30+
- `poetry`-managed, Python 3.8+; `poetry install --all-extras` has run on the
31+
runner, and the live-warehouse connection env is set — so both
32+
`poetry run python -m pytest tests/unit` (mocked) AND `tests/e2e` (live) are
33+
runnable. If a reviewer's ask is about connector behavior that only an E2E test
34+
can verify, add/adjust a `tests/e2e` test on the RIGHT backend for the change
35+
(Thrift default / `use_sea=True` / `use_kernel=True` — see CONTRIBUTING.md →
36+
"Backends and test tiers"), don't settle for a mocked unit test.
3337
- Source is under `src/databricks/sql/`; unit tests under `tests/unit/`.
3438
Follow `CONTRIBUTING.md`: PEP 8 with a 100-char line limit, type hints where
3539
the surrounding code uses them. This is a widely-consumed connector — keep

.bot/prompts/engineer/system.md

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -97,11 +97,24 @@ up whatever it needs.
9797

9898
== HOW TO WORK (bug-fix flow) ==
9999

100+
0. **Pick the BACKEND the bug is on — reproduce on that one.** The connector has
101+
three backends and a bug on one won't reproduce on another. Choose from the
102+
issue:
103+
- **kernel** (`use_kernel=True`) — only if the issue is specifically about the
104+
Rust kernel / `use_kernel`. Repro in `tests/e2e/test_kernel_backend.py` and run
105+
it alone (it needs the real wheel; see the `-m "not realkernel"` note above).
106+
- **SEA** (`use_sea=True`) — if the issue implicates SEA, or the area is
107+
backend-parametrized (add the `{"use_sea": True}` param case).
108+
- **Thrift** (the default, no kwarg) — everything else; this is the common case.
109+
See CONTRIBUTING.md → "Backends and test tiers" for the full matrix (selection
110+
kwarg + where each backend's tests live).
111+
100112
1. **Write the failing E2E test FIRST — before you deep-dive the fix.** Your first
101-
substantive action is a `tests/e2e/` test that REPRODUCES the bug. Do only the
102-
minimal reading needed to write it (find the API to call + how the e2e tests
103-
connect). Run it with `-k` and confirm it **fails for the right reason** (the
104-
bug — not a compile/setup/skip). A *skipped* test is not a reproduction.
113+
substantive action is a `tests/e2e/` test (on the backend from step 0) that
114+
REPRODUCES the bug. Do only the minimal reading needed to write it (find the API
115+
to call + how the e2e tests connect). Run it with `-k` and confirm it **fails for
116+
the right reason** (the bug — not a compile/setup/skip). A *skipped* test is not
117+
a reproduction.
105118
- **Reproduction is a HARD GATE.** If after a focused effort (a few attempts,
106119
not dozens) you cannot get a test that fails for the right reason — it only
107120
skips, you can't reach the warehouse, or you can't trigger the bug — **STOP

CONTRIBUTING.md

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,33 @@ The `PySQLStagingIngestionTestSuite` namespace requires a cluster running DBR ve
144144

145145
The suites marked `[not documented]` require additional configuration which will be documented at a later time.
146146

147+
#### Backends and test tiers
148+
149+
The connector has **three execution backends**, selected per connection. When you
150+
reproduce or fix a bug, use the backend the bug is actually on — a Thrift bug won't
151+
reproduce on a kernel connection, and vice versa:
152+
153+
| Backend | Select via (connect kwarg / `extra_params`) | Where its tests live |
154+
| --- | --- | --- |
155+
| **Thrift** (default) | *(nothing — the default path)* | the general `tests/e2e` suite (the `{}` parametrize case) and mocked `tests/unit` |
156+
| **SEA** | `use_sea=True` | the general `tests/e2e` suite (the `{"use_sea": True}` parametrize case) |
157+
| **Kernel** (Rust, optional) | `use_kernel=True` | the dedicated `tests/e2e/test_kernel_backend.py` / `test_kernel_tls.py`, plus the offline routing test `tests/unit/test_session.py -m realkernel` |
158+
159+
Notes that matter when running the suite:
160+
161+
- **Kernel is an opt-in extra**, not part of the default install. `use_kernel=True`
162+
needs `pip install "databricks-sql-connector[kernel]"` (or `poetry install
163+
--all-extras`); without it the connector raises a clear "install the `[kernel]`
164+
extra" error. Most bugs are on the **Thrift/SEA** path — reproduce those there;
165+
only reach for kernel when the issue is specifically about `use_kernel`.
166+
- **`realkernel` tests must run in their own pytest invocation.** When the real
167+
kernel wheel is installed (`--all-extras`), several unit tests fake
168+
`databricks_sql_kernel` in `sys.modules`; the `@pytest.mark.realkernel` guard test
169+
detects that shadowing and **fails loudly**. So a *broad* unit run with the wheel
170+
present must **deselect it**: `poetry run python -m pytest tests/unit -m "not
171+
realkernel"`, and run the real-wheel tests separately with `-m realkernel` (this is
172+
exactly how CI splits them — see `code-coverage.yml` / `code-quality-checks.yml`).
173+
147174

148175
### Code formatting
149176

0 commit comments

Comments
 (0)