Repository navigation
[WIP] Wait for and print the results of databricks pipelines test - #7298
tharunbhuk-db wants to merge 19 commits into
Conversation
2c84257 to
8542365
Compare
|
Commit: Regression test reportSelected 9 changed tests (limit=20, 0 not selected) 9 tests: regression=9 unreleased=0 coverage=0 failing=0
TestAccept/pipelines/databricks-cli-help: branch ✅ | main (aed6702) ❌ | latest (v1.23.0) ❌main (aed6702): latest (v1.23.0): same output as main (aed6702) TestAccept/pipelines/test/arguments: branch ✅ | main (aed6702) ❌ | latest (v1.23.0) ❌main (aed6702): latest (v1.23.0): same output as main (aed6702) TestAccept/pipelines/test/immutable: branch ✅ | main (aed6702) ❌ | latest (v1.23.0) ❌main (aed6702): latest (v1.23.0): same output as main (aed6702) TestAccept/pipelines/test/many-cases: branch ✅ | main (aed6702) ❌ | latest (v1.23.0) ❌main (aed6702): latest (v1.23.0): same output as main (aed6702) TestAccept/pipelines/test/pipeline-id: branch ✅ | main (aed6702) ❌ | latest (v1.23.0) ❌main (aed6702): latest (v1.23.0): same output as main (aed6702) TestAccept/pipelines/test/results: branch ✅ | main (aed6702) ❌ | latest (v1.23.0) ❌main (aed6702): latest (v1.23.0): same output as main (aed6702) TestAccept/pipelines/test/run-control: branch ✅ | main (aed6702) ❌ | latest (v1.23.0) ❌main (aed6702): latest (v1.23.0): same output as main (aed6702) TestAccept/pipelines/test/selection: branch ✅ | main (aed6702) ❌ | latest (v1.23.0) ❌main (aed6702): latest (v1.23.0): same output as main (aed6702) TestAccept/pipelines/test/submit-error: branch ✅ | main (aed6702) ❌ | latest (v1.23.0) ❌main (aed6702): latest (v1.23.0): same output as main (aed6702) |
Integration test reportCommit: 23e0bb0
Top 17 slowest tests (at least 2 minutes):
|
8542365 to
cb0a526
Compare
cb0a526 to
025649f
Compare
… also when the update completed
… not report test_only
The command validated --resume with github.com/google/uuid, which made it a direct dependency without a license annotation. cmd/pipelines/history.go already uses the standard library package.
Unit tests keep the edge cases that the fake service cannot produce; the acceptance scenarios own the outcomes, output, resume and conflict paths.
…needs root_path With --no-wait and only workspace paths, nothing uses root_path, so a failed read should not stop the run.
…eads the pipeline first
macOS runners use bash 3.2, which has no &>> redirect. The calls with workspace or API paths that this PR adds also need Windows path conversion turned off, like the ones before them. Co-authored-by: Isaac <no-reply@databricks.com>
025649f to
23e0bb0
Compare
| if incompleteReason == "" { | ||
| summary := run.summaries[0] | ||
| testsFailed := summary.Total == 0 || summary.Failed > 0 || summary.Errors > 0 | ||
| if !testsFailed && state == pipelines.UpdateInfoStateCompleted { |
There was a problem hiding this comment.
A COMPLETED update whose tests all passed returns nil here without checking run.fatalError, so an update that also reported a fatal error exits 0. Could we add run.fatalError == "" to this condition, with a TestTestRunError case for it?
Related: a run where every test is SKIPPED also returns nil here. That matches the design (an all-skipped run completes), but CI and agents will read it as green, so the help should say so explicitly.
| UpdateId: tw.updateID, | ||
| }) | ||
| if err != nil { | ||
| return "", err |
There was a problem hiding this comment.
A GetUpdate error (and the same for the events read just below) ends the wait with the raw error and no --resume hint, even though the update keeps running. The SDK client already retries transient HTTP errors, so no extra retry logic is needed here. Could the error just include the same resume command as the timeout path?
| updateID := resumeUpdateID | ||
| if !resume { | ||
| req := newStartTestUpdateRequest(target.selections, pytestArgs) | ||
| updateID, err = startTestUpdate(ctx, target.w, target.pipelineID, req) |
There was a problem hiding this comment.
This POST uses the signal context, so a Ctrl+C during Start Update loses the update ID, and the update runs with no way to find it from this output. Could the POST use cmd.Context()?
Also on this line: when the service rejects the start because an update is already active, a short static hint after its message would give agents the next step, for example "Run 'databricks pipelines stop KEY' to cancel it, or wait for it to finish." There's no need to parse the ID out of the message.
| fmt.Fprintln(out) | ||
|
|
||
| if incompleteReason != "" { | ||
| fmt.Fprintf(out, "RUN INCOMPLETE: %s.\n", incompleteReason) |
There was a problem hiding this comment.
The most likely cause of INCOMPLETE is events still arriving after the terminal state, and the update can still be read later. Could we print the --resume command here too?
|
|
||
| // testVerdictErrorPrefix starts the message of the error by which the service reports that tests | ||
| // failed, such as "[PIPELINE_TEST_FAILED] 1 of the pipeline's tests failed (1 passed, 0 skipped).". | ||
| const testVerdictErrorPrefix = "[PIPELINE_TEST_FAILED]" |
There was a problem hiding this comment.
Recognizing the service verdict by message prefix is fragile. The exit code doesn't depend on it, only the output does. The SDK's ErrorDetail has no error class yet, so a comment saying this is temporary until a class is exposed would help.
| // update ended in another way or reports an error of its own. The service's own verdict, | ||
| // such as "[PIPELINE_TEST_FAILED] 1 of the pipeline's tests failed", is not. | ||
| ended := state == pipelines.UpdateInfoStateFailed || state == pipelines.UpdateInfoStateCompleted | ||
| if testsFailed && ended && run.fatalError == "" { |
There was a problem hiding this comment.
Every failure path exits 1. Test failures and zero-collected runs print nothing to stderr, while infra failures print Error:, so an agent tells "fix the code" from "retry" only from the output. pipelines run behaves the same, so distinct exit codes would be a CLI-wide decision. For this stack, could the help just say what exit 0 and 1 mean? Small related nit: with --junit-xml, the JUnit report: line prints after the summary, so tail -1 misses the summary; printing it before the summary would keep the summary last.
| summary := run.summaries[0] | ||
| switch summary.Total { | ||
| case 0: | ||
| fmt.Fprintln(out, "No tests were collected.") |
There was a problem hiding this comment.
Agents hit this often, and the message gives no hint. Naming what was run would make it self-correcting, e.g. No tests were collected from <testfile>. Check the test selection and any --pytest-arg filters. (I'd avoid restating pytest's naming rules here, since projects can change them in their pytest configuration.) But if we can give a bit better message that would be nice.
| testCasePassed testCaseStatus = "PASSED" | ||
| testCaseFailed testCaseStatus = "FAILED" | ||
| testCaseSkipped testCaseStatus = "SKIPPED" | ||
| testCaseError testCaseStatus = "ERROR" |
There was a problem hiding this comment.
should this be errored? I see we do error and errored at some places would be good if we can do consistent.
| @@ -0,0 +1,799 @@ | |||
| # 200 cases emit 601 events, more than two pages of List Pipeline Events. | |||
There was a problem hiding this comment.
Non-blocking: instead of committing 800 lines of identical tests, could the script generate this file before pipelines deploy? The fake collects tests with a regex over the deployed files (libs/testserver/pipelines_testing.go:106), so this works the same and runs on bash 3.2 and Git Bash:
for i in $(seq 0 199); do printf 'def test_case_%03d():\n assert True\n\n' "$i"; done > tests/test_many.pyAlso worth checking how many cases you actually need. The CLI doesn't send max_results, so the fake pages at its default of 25 events (pipelines_testing.go:332), and the output shows 8 pages read. Fewer cases may cover the same paths. The page math reads better in the script than as a comment in a generated file.
| assert.Equal(t, 400, response.StatusCode) | ||
| } | ||
|
|
||
| func TestPipelineStartUpdateRejectsInvalidTestOptions(t *testing.T) { |
There was a problem hiding this comment.
Non-blocking: this file tests the fake more deeply than the CLI contract needs, and a few could go:
TestPipelineStartUpdateRejectsInvalidTestOptionsandTestPipelineStartUpdateRejectsTestOptionsWithoutTestOnly, together with the fake'spipelineTestOptionsError. The CLI validates--pytest-argand makes selections absolute before any request, and it always sendstest_only: true, sopipelines testnever reaches this validation.TestPipelineFiniteUpdateIsActiveUntilItEnds,TestPipelineUpdateThatIsNeverPolledEndsWithItsFirstStateandTestPipelineStopHidesEventsOfStatesNotReached. They pin the fake's state timing, and what the CLI sees (409, CANCELED) is golden inrun-controlandsubmit-error.TestPipelineEventsPages. Paging is covered by the CLI's own reader test andmany-cases.
TestCollectPipelineTests, TestPipelineTestUpdate and TestPipelineTestUpdateConflictAndStop cover what the acceptance tests rely on. Also, line 242 repeats the assertion on line 241.
🥞 Stacked PR
Design doc: https://docs.google.com/document/d/1atW-JG_hQu-G-YX-m1M7OodNZdH7LMc9H7CBlQAv5gw/edit
Changes
--no-wait,--resume <update-id>and--timeout. A timeout or Ctrl+C ends only the local wait and prints the--resumecommand.--resumerejects a value that is not an update ID, and an update that the service reports is not a test update.RUN INCOMPLETE: <reason>when results are missing.Why
CI needs the results in the log, and an exit code that fails the build when a test fails. The events are read with a raw call, because the Go SDK does not have their test fields yet, and its iterator repeats
order_bywithpage_token, which the service rejects.Tests
acceptance/pipelines/test/{results,run-control,many-cases,pipeline-id,selection,immutable,arguments,submit-error}.This pull request and its description were written by Isaac.