Repository navigation
[WIP] Add the testing property of pipelines to bundles - #7300
tharunbhuk-db wants to merge 10 commits into
Conversation
cf934e0 to
cc2e61a
Compare
52e5f6b to
9c9d9c8
Compare
|
Commit: Regression test reportSelected 16 changed tests (limit=20, 0 not selected) 22 tests: regression=22 unreleased=0 coverage=0 failing=0
TestAccept/bundle/environments/editable-folder/DMS=: branch ✅ | main (aed6702) ❌ | latest (v1.23.0) ❌main (aed6702): latest (v1.23.0): same output as main (aed6702) TestAccept/bundle/environments/editable-folder/DMS=true: branch ✅ | main (aed6702) ❌ | latest (v1.23.0) ❌main (aed6702): latest (v1.23.0): same output as main (aed6702) TestAccept/bundle/refschema/DMS=: branch ✅ | main (aed6702) ❌ | latest (v1.23.0) ❌main (aed6702): latest (v1.23.0): same output as main (aed6702) TestAccept/bundle/refschema/DMS=true: branch ✅ | main (aed6702) ❌ | latest (v1.23.0) ❌main (aed6702): latest (v1.23.0): TestAccept/bundle/resources/pipelines/testing-dependencies/DMS=: branch ✅ | main (aed6702) ❌ | latest (v1.23.0) ❌main (aed6702): latest (v1.23.0): same output as main (aed6702) TestAccept/bundle/resources/pipelines/testing-dependencies/DMS=true: branch ✅ | main (aed6702) ❌ | latest (v1.23.0) ❌main (aed6702): latest (v1.23.0): same output as main (aed6702) TestAccept/bundle/resources/pipelines/testing-disabled/DMS=: branch ✅ | main (aed6702) ❌ | latest (v1.23.0) ❌main (aed6702): latest (v1.23.0): same output as main (aed6702) TestAccept/bundle/resources/pipelines/testing-disabled/DMS=true: branch ✅ | main (aed6702) ❌ | latest (v1.23.0) ❌main (aed6702): latest (v1.23.0): same output as main (aed6702) TestAccept/bundle/resources/pipelines/testing-same-name-libraries/DMS=: branch ✅ | main (aed6702) ❌ | latest (v1.23.0) ❌main (aed6702): latest (v1.23.0): same output as main (aed6702) TestAccept/bundle/resources/pipelines/testing-same-name-libraries/DMS=true: branch ✅ | main (aed6702) ❌ | latest (v1.23.0) ❌main (aed6702): latest (v1.23.0): same output as main (aed6702) TestAccept/bundle/resources/pipelines/testing/DMS=: branch ✅ | main (aed6702) ❌ | latest (v1.23.0) ❌main (aed6702): latest (v1.23.0): same output as main (aed6702) TestAccept/bundle/resources/pipelines/testing/DMS=true: branch ✅ | main (aed6702) ❌ | latest (v1.23.0) ❌main (aed6702): latest (v1.23.0): TestAccept/pipelines/databricks-cli-help: branch ✅ | main (aed6702) ❌ | latest (v1.23.0) ❌main (aed6702): latest (v1.23.0): same output as main (aed6702) 9 more failure details omitted, see the full report in the run. |
Integration test reportCommit: 286aa60
Top 12 slowest tests (at least 2 minutes):
|
cc2e61a to
b4df318
Compare
9c9d9c8 to
742ef87
Compare
…E_EXPERIMENTAL_PIPELINE_TESTING
… testing ones beside them
… changes Co-authored-by: Isaac <no-reply@databricks.com>
e038a13 to
e895d35
Compare
b4df318 to
7c61f68
Compare
…t create pipelines Co-authored-by: Isaac <no-reply@databricks.com>
Reading a directory fails with "is a directory" on Linux and macOS but with "Incorrect function." on Windows. Co-authored-by: Isaac <no-reply@databricks.com>
Turn off Git Bash path conversion only for the API calls with "/" paths, so the Python helpers keep working, and write library paths with forward slashes as the other same-name library case does. Co-authored-by: Isaac <no-reply@databricks.com>
| skipRewrite: noSkipRewrite, | ||
| }, | ||
| { | ||
| pattern: base.Append(dyn.Key("testing"), dyn.Key("sources"), dyn.AnyIndex(), dyn.Key("glob"), dyn.Key("include")), |
There was a problem hiding this comment.
Question on scope: this PR validates testing.environment.dependencies client-side, but not testing.sources. The service checks sources (workspace root, .py or /**, no .., no overlap with libraries) and bundle deploy reports its error. Relying on the service is reasonable and keeps the CLI smaller, but could we pick one approach for both settings? Either the service is the source of truth for both (and the dependency validator could go, so the two can't drift), or both are validated here.
|
|
||
| // With pipeline testing enabled, the path after -e can also be a project folder, with optional | ||
| // extras such as ./project[test]. | ||
| if opts.Mode == paths.TranslateModeEnvironmentPipFlag && flag == "-e" && env.ExperimentalPipelineTesting(ctx) { |
There was a problem hiding this comment.
The -e ./folder mapping is gated by the pipeline-testing flag, but editable-project-folder.md calls it a fix, and job environments take the same path (job_libraries_paths_visitor.go). If it restores intended behavior, it shouldn't need the flag. If it stays gated, could the changelog say so and mention jobs?
Related: since this mapping is its own behavior change, and covers job environments too, it might be easier as a small separate PR together with acceptance/bundle/environments/editable-folder and TestTranslatePathEditableProjectFolder. That takes about 10 files out of this PR and lets the gating question be settled on its own.
| } | ||
| // Path translation rewrites a path in the bundle after "-r " or "-e ", so a relative path that | ||
| // remains after an option was not rewritten. | ||
| case option != "" || !libraries.IsLibraryLocal(filePath): |
There was a problem hiding this comment.
--editable ./project[test] lands here as "not a file", which doesn't tell users what to do. Could the message say to use -e ./project[test], or could --editable be translated like -e? Also, strings.Contains(filePath, " -") at line 86 rejects valid paths such as /Workspace/a -b/project; checking only for a leading option would avoid that.
| if config.Testing != nil { | ||
| return nil, r.doWithTesting(ctx, http.MethodPut, "/api/2.0/pipelines/"+id, request, config.Testing, nil) | ||
| } | ||
| return nil, r.client.Pipelines.Update(ctx, request) |
There was a problem hiding this comment.
Removing testing goes through this SDK PUT, which sends no testing field, so clearing it relies on the service replacing spec.testing on PUT. Could we verify on staging that a deploy removes it, and that bundle plan shows 0 changes for both a testing and a plain pipeline with the flag on?
| "$fields": | ||
| "dependencies": | ||
| "description": |- | ||
| A list of files that declare the packages the tests need: a project folder, a requirements file, or a wheel, in the workspace or a UC volume. A project folder can take `--editable` and an extra, for example `--editable ${workspace.file_path}[test]`. Package names and URLs are rejected. |
There was a problem hiding this comment.
Public docs text:
- Bundle paths such as
-e ./project[test]and-r ./tests/requirements.txtalso work, and they're relative to the YAML file that defines them. Worth saying. - The
globdescription should state the rules: a.pyfile or a folder followed by/**, no other wildcards, no overlap withlibraries. - Mention that it needs
DATABRICKS_BUNDLE_ENABLE_EXPERIMENTAL_PIPELINE_TESTING.
| if message != "" { | ||
| return Response{ | ||
| StatusCode: 400, | ||
| Body: map[string]string{"error_code": "INVALID_PARAMETER_VALUE", "message": message}, |
There was a problem hiding this comment.
Nit: the fake returns INVALID_PARAMETER_VALUE for a pipeline with no test sources. The service's class for this is PIPELINE_TEST_NO_TEST_SOURCES, and matching it lets CLI tests catch a mismatch. Related: when the bundle YAML has testing but the deployed pipeline doesn't, this error could tell users to run databricks pipelines deploy.
| Files: 5 uploaded, 0 deleted | ||
| Resources: 1 created, 0 changed, 0 deleted, 0 unchanged | ||
|
|
||
| >>> print_requests.py //api/2.0/pipelines --nostamp |
There was a problem hiding this comment.
Non-blocking: the three full request dumps and the 47-line plan JSON pin the whole pipeline body (channel, edition, libraries, metadata path) to check a single testing field. They'll need updating whenever an unrelated default changes. Piping through jq '.body.testing' and jq '.changes.testing', as the validate step above already does, keeps the same assertions with about 90 fewer golden lines.
| at resources.pipelines.orders.testing.environment.dependencies[3] | ||
| in databricks.yml:19:15 | ||
|
|
||
| A test dependency is a project folder, a wheel, or a requirements file after -r, in the bundle, under /Workspace/, or under /Volumes/. Declare test packages in a project file, such as the test extra of a pyproject.toml, and reference its folder, for example -e ./project[test]. For a path in the bundle, use -e or -r followed by a space. URLs, other pip options, and .. are not supported. |
There was a problem hiding this comment.
Non-blocking: each of the five errors repeats the same three-line explanation, and which inputs are accepted or rejected is already unit-tested in pipeline_testing_dependencies_test.go. One or two errors here would pin the message text, with less prose to keep in sync.
| assert.Empty(t, workspace.collectPipelineTests(nil)) | ||
| } | ||
|
|
||
| func TestPipelineTestTargets(t *testing.T) { |
There was a problem hiding this comment.
Non-blocking: TestPipelineTestTargets and TestPipelineEditReplacesTestSources test the fake's own rules. The behavior they guard is already golden in pipeline-id, selection (the outside-sources and no-sources messages) and bundle/resources/pipelines/testing (removing testing on edit). Both could go.
| @@ -0,0 +1 @@ | |||
| trace musterr $CLI bundle validate | |||
There was a problem hiding this comment.
Non-blocking: this case is a single bundle validate with the gate off, and it takes a whole directory (6 files). It could be a step in testing/script that reuses that bundle, for example trace musterr env -u DATABRICKS_BUNDLE_ENABLE_EXPERIMENTAL_PIPELINE_TESTING $CLI bundle validate. (testing-same-name-libraries does need its own conflicting config, so keeping that one separate makes sense.)
🥞 Stacked PR
Design doc: https://docs.google.com/document/d/1atW-JG_hQu-G-YX-m1M7OodNZdH7LMc9H7CBlQAv5gw/edit
Changes
resources.pipelines.<key>.testing, with the test sources and test-only dependencies thatdatabricks pipelines testruns with.DATABRICKS_BUNDLE_ENABLE_EXPERIMENTAL_PIPELINE_TESTING=true. While the variable is off,bundle validaterejectstestingand names the variable, and bundles deploy exactly as before: pipelines are read with the SDK, and the existing path rules are unchanged.testingare translated likelibraries.bundle validatecatches common mistakes in test dependencies, such as package names, URLs and a requirements file without-r.bundle planshows changes totestingmade outside the bundle, andbundle deployundoes them.-e, such as-e ./project[test], maps to its deployed folder instead of failing.Why
A bundle needs to declare where a pipeline's tests and their dependencies live. The CLI uses its own types and raw calls for
testing, because the Go SDK does not have the field yet. Depends on API support fortestingon Create and Edit. Keep as draft until then.Tests
acceptance/bundle/resources/pipelines/testing*(testing-disabledcovers the variable unset),acceptance/bundle/environments/editable-folderandacceptance/pipelines/test. Thelivetest also installs a local test dependency.This pull request and its description were written by Isaac.