Repository navigation
Add the databricks pipelines test command - #7297
Conversation
|
Commit: Regression test reportSelected 4 changed tests (limit=20, 0 not selected) 4 tests: regression=4 unreleased=0 coverage=0 failing=0
TestAccept/pipelines/databricks-cli-help: branch ✅ | main (688c228) ❌ | latest (v1.25.0) ❌main (688c228): latest (v1.25.0): same output as main (688c228) TestAccept/pipelines/test/arguments: branch ✅ | main (688c228) ❌ | latest (v1.25.0) ❌main (688c228): latest (v1.25.0): same output as main (688c228) TestAccept/pipelines/test/pipeline-id: branch ✅ | main (688c228) ❌ | latest (v1.25.0) ❌main (688c228): latest (v1.25.0): same output as main (688c228) TestAccept/pipelines/test/submit-error: branch ✅ | main (688c228) ❌ | latest (v1.25.0) ❌main (688c228): latest (v1.25.0): same output as main (688c228) |
✅ Integration test reportCommit: 62049a1
Top 3 slowest tests (at least 2 minutes):
|
| return nil, fmt.Errorf("test selection %q has no path", selection) | ||
| } | ||
| if strings.HasPrefix(p, "/") { | ||
| p = path.Clean(p) |
There was a problem hiding this comment.
path.Clean quietly turns /Workspace/x/../y into a different path than the user typed, and path.Join in rootPathSelection does the same for relative selections. Could we pass selections through as typed instead? The service already rejects .. and wildcards with its own error, so dropping the normalization is less code and keeps the error about what the user actually wrote. The test at test_test.go:53 would then expect the input unchanged.
There was a problem hiding this comment.
Done. Selections now pass through as typed, with no path.Clean or path.Join.
| err = apiClient.Do( | ||
| ctx, | ||
| http.MethodPost, | ||
| fmt.Sprintf("/api/2.0/pipelines/%s/updates", pipelineID), |
There was a problem hiding this comment.
--pipeline-id goes into the URL path unescaped. Could we validate the ID format, or url.PathEscape it?
There was a problem hiding this comment.
Done. --pipeline-id is checked with uuid.Parse, as pipelines history does.
| // resolveTestTargetByID identifies the pipeline by --pipeline-id; all positional arguments are selections. | ||
| func resolveTestTargetByID(cmd *cobra.Command, args []string, pipelineID string) (testTarget, error) { | ||
| if cmd.Flag("target").Value.String() == "" { | ||
| cmd.SetContext(root.SkipLoadBundle(cmd.Context())) |
There was a problem hiding this comment.
Question: with --pipeline-id inside a project, the bundle isn't loaded, so the bundle's host and profile are ignored, and the command can talk to a different workspace than pipelines run would. Is that intended?
There was a problem hiding this comment.
Yes, intended. It follows pipelines history <id>: with an ID and no --target, the bundle isn't loaded. Pass --target to use the bundle's workspace.
| cmd := &cobra.Command{ | ||
| Use: "test [PIPELINE_KEY] [TEST_SELECTION...] [flags] [--pytest-arg=ARG ...]", | ||
| Short: "*Preview* Run the tests of a pipeline", | ||
| Long: `Run the tests of a pipeline in a test-only update. |
There was a problem hiding this comment.
Public text:
- Short says "Preview" while Long and the gate say "experimental". Could we pick one label and name the env var in the help?
- Other strings:
- "outside a project" → "Run this command from a bundle directory, or pass --pipeline-id."
- "synchronized bundle files" → "%s is not in the files synced from %s; pass its /Workspace path."
- The immutable error → "Relative test paths need synced local files, but workspace.sync_mode is immutable; pass the /Workspace path."
There was a problem hiding this comment.
I'll do this in a follow-up.
| option, value := arg[:2], strings.TrimPrefix(arg[2:], "=") | ||
| return fmt.Errorf("pytest argument %q must be split: pass --pytest-arg=%s --pytest-arg=%s", arg, option, shellquote.BashArg(value)) | ||
| case !strings.HasPrefix(arg, "-"): | ||
| return fmt.Errorf("unsupported pytest argument %q: pass test paths and test IDs as TEST_SELECTION arguments", arg) |
There was a problem hiding this comment.
Nit: --pytest-arg="" lands here and reports unsupported pytest argument "": pass test paths..., which is misleading for an empty value. Since this flag's rules are the CLI's own, an explicit empty-value message would help. Also nit: the Use string puts selections before flags, while sibling commands use run [flags] [KEY].
There was a problem hiding this comment.
Done. An empty value now gets its own error, and the usage is test [flags] [TEST_SELECTION...].
| ctx := cmd.Context() | ||
|
|
||
| if _, err := resources.Lookup(b, key, isPipeline); err != nil { | ||
| return testTarget{}, fmt.Errorf("%w; the first argument must be a pipeline KEY when test selections are given", err) |
There was a problem hiding this comment.
In a one-pipeline project, pipelines test tests/test_orders.py takes the path as the KEY. The rule is fine, but the error could name the pipeline and show the working command, so an agent can self-correct in one step: "tests/test_orders.py" is not a pipeline in this bundle (pipelines: my_pipeline). Usage: databricks pipelines test my_pipeline tests/test_orders.py.
There was a problem hiding this comment.
Done in #7431. The error lists the bundle's pipelines and shows the working command.
| and -m are supported, each followed by its value in the next --pytest-arg, for | ||
| example: | ||
|
|
||
| databricks pipelines test my_pipeline --pytest-arg=-k --pytest-arg='orders and not slow'`, |
There was a problem hiding this comment.
Agents and CI scripts learn the command from --help, which has one example. Could the Long text cover the common flows and the outcome contract?
- Examples: run all tests of a pipeline; run one file; re-run one failing test by ID; CI with
--junit-xml; resume after--timeout. - A short exit-code paragraph: when it exits 0, and that failures, incomplete runs and errors exit 1.
- Both env vars needed while the feature is experimental.
There was a problem hiding this comment.
The exit codes and the env var are in the help from #7298. I'll add more examples in a follow-up.
66e7f7e to
62049a1
Compare
62049a1 to
4401851
Compare
|
Follow-ups from review:
|
Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
The command starts a test-only update of the pipeline named by --pipeline-id and prints its IDs. Selections are workspace paths or paths relative to the pipeline's root_path, and are sent as typed so that the service checks them. --pytest-arg accepts only -k and -m. The command is experimental and runs only with DATABRICKS_CLI_PIPELINES_TEST=true. Co-authored-by: Isaac <no-reply@databricks.com>
4401851 to
9be85fd
Compare
Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
pranshupand-db
left a comment
There was a problem hiding this comment.
Thanks Tharun for re-splitting the stack and working through every thread. The smaller PRs made this much easier to review. LGTM for the whole stack (#7297 → #7431 → #7432 → #7433 → #7298 → #7426 → #7299 → #7434).
What I checked:
- No false pass. Exit 0 requires all of these: a COMPLETED update, exactly one summary, one final result per case that matches the summary, at least one case, nothing failed or errored, and no fatal error from the update. The earlier blockers are fixed:
fatalErroron success, Ctrl+C during Start Update, atomic JUnit writes,[in IDs, and--pipeline-idvalidation. - Smoke run. I built #7434 (
8b690b3) and ran it against dogfood staging with--pipeline-id. It handled pass/fail/skip/xfail/fixture/teardown/parametrized outcomes, a passing selection, zero collected, an unknown test ID,--timeoutthen--resume,--no-wait, and a second start while one is active. Every case gave the expected exit code and JUnit report. Example 1 passes 5/5.
Merge note: #7297 through #7433 return after Start Update without waiting, so let's land the stack together before a CLI release cuts between them. The gate covers it, but it's cleaner that way.
Follow-ups (not blocking, can you track them in a ticket?):
- Headline user errors by their error class instead of "RUN INCOMPLETE". Today an unknown test ID (
PIPELINE_TEST_RUN_REFUSED) or an import error in a test file reads as incomplete, so agents and CI retry instead of fixing the test. Still reproduces on8b690b3. - Add next-step hints: the
--resumecommand on INCOMPLETE and on a failed GetUpdate/events read, a next step for "an active update already exists" (--resume <id>orpipelines stop;pipelines runhas--restart), and the selection/filters in "No tests were collected". - Agree on one rule with the pytest plugin (universe #2932592) for a test case pytest runs twice. The plugin sends a second final status, which
incompleteReasonrejects as "has more than one result" (test_events.go:284). - Help and public text: more examples in
--help, one "Preview"/"experimental" label, and the deploy recommendation (it namespipelines deployin bundles and prints on every run). - JUnit: stable classnames with
sync_mode: immutable, one naming scheme for the synthetic entries, and skip/xfail reasons in<skipped message=…>. - The output is now dbt-style (
1 of 11 PASSED …,Done. PASSED=4 FAILED=3 … TOTAL=11), while the design doc and the bugbash doc show11 tests: 4 passed, 3 failed, …. Fine if that was intentional; let's update both docs to match.
The data-plane items from the bugbash (--assert plain, redacted paths, ANSI codes, captured stdout) are tracked in the findings doc, separate from this stack.
Co-authored-by: Isaac <no-reply@databricks.com>
❌ Integration test reportCommit: 4528983
14 interesting tests: 10 flaky, 4 FAIL
Top 50 slowest tests (at least 2 minutes):
|
## 🥞 Stacked PR - [stack/pipelines-test-pr1-command](databricks#7297) [[Files changed](https://github.com/databricks/cli/pull/7297/files)] - [**stack/pipelines-test-bundle-key**](databricks#7431) [[Files changed](https://github.com/databricks/cli/pull/7431/files)] ← _this PR_ - [stack/pipelines-test-read-events](databricks#7432) [[Files changed](https://github.com/databricks/cli/pull/7432/files)] - [stack/pipelines-test-print-results](databricks#7433) [[Files changed](https://github.com/databricks/cli/pull/7433/files)] - [stack/pipelines-test-pr2-output](databricks#7298) [[Files changed](https://github.com/databricks/cli/pull/7298/files)] - [stack/pipelines-test-a4-run-control](databricks#7426) [[Files changed](https://github.com/databricks/cli/pull/7426/files)] - [stack/pipelines-test-pr3-junit](databricks#7299) [[Files changed](https://github.com/databricks/cli/pull/7299/files)] - [stack/pipelines-test-junit-flag](databricks#7434) [[Files changed](https://github.com/databricks/cli/pull/7434/files)] --------- - In a bundle, identify the pipeline by KEY: `pipelines test [flags] [KEY] [TEST_SELECTION...]`. - Relative selections are local paths, mapped to their deployed paths. - A first argument that is not a KEY gets an error that names the bundle's pipelines and the working command. Tests: unit tests; acceptance in `acceptance/pipelines/test/{selection,immutable}`. This pull request and its description were written by Isaac. Co-authored-by: Isaac <no-reply@databricks.com>
🥞 Stacked PR
Use this link to review incremental changes.
databricks pipelines test --pipeline-id <id> [TEST_SELECTION...]: starts a test-only update and prints its IDs.DATABRICKS_CLI_PIPELINES_TEST=true.root_path, sent as typed.--pytest-argaccepts only-kand-m.Tests: unit tests; acceptance in
acceptance/pipelines/test/{arguments,pipeline-id,submit-error}.This pull request and its description were written by Isaac.