Skip to content

acc: Use BundleConfig to define bundle.name once - #2845

Merged
denik merged 4 commits into
mainfrom
denik/acc-cleanup-bundle-section
May 21, 2025
Merged

acc: Use BundleConfig to define bundle.name once#2845
denik merged 4 commits into
mainfrom
denik/acc-cleanup-bundle-section

Conversation

@denik

@denikdenik commented May 9, 2025

Copy link
Copy Markdown
Contributor

Changes

Using BundleConfig option (#2809), specify boilerplate bundle.name once and clean up individual test cases.

The tests with diagnostics (lineno, column) are left alone.

Test acceptance/bundle/volume_path is split into invidual tests, one databricks.yml per test (since BundleConfig only updates databricks.yml[.tmpl] in the root of the test.

Why

Simpler to read and write, make each test focussed on what's important.

Tests

Existing tests.

Benchmarked acceptance test suite (local) to ensure this does not add a lot of overhead (due to parsing/marshalling every config):

This branch: Time (mean ± σ): 18.364 s ± 0.857 s [User: 32.707 s, System: 27.281 s]
Range (min … max): 17.002 s … 19.922 s 10 runs
Main:
Time (mean ± σ): 18.337 s ± 0.789 s [User: 32.613 s, System: 27.235 s]
Range (min … max): 17.078 s … 19.342 s 10 runs

@denik
deniktemporarily deployed to test-trigger-is May 9, 2025 14:38 — with GitHub Actions Inactive
@denik
denik changed the base branch from main to denik/acc-bundle-configMay 9, 2025 14:38
@denik
deniktemporarily deployed to test-trigger-is May 9, 2025 14:41 — with GitHub Actions Inactive
@denikdenik mentioned this pull request May 9, 2025
@denik
denik marked this pull request as ready for review May 9, 2025 14:46
@denik
deniktemporarily deployed to test-trigger-is May 9, 2025 14:52 — with GitHub Actions Inactive
@denik
deniktemporarily deployed to test-trigger-is May 9, 2025 15:00 — with GitHub Actions Inactive
@denik
deniktemporarily deployed to test-trigger-is May 9, 2025 15:03 — with GitHub Actions Inactive
@denik
deniktemporarily deployed to test-trigger-is May 9, 2025 15:06 — with GitHub Actions Inactive
@denik
deniktemporarily deployed to test-trigger-is May 9, 2025 15:10 — with GitHub Actions Inactive
@denik
deniktemporarily deployed to test-trigger-is May 9, 2025 15:11 — with GitHub Actions Inactive
@denik
deniktemporarily deployed to test-trigger-is May 9, 2025 15:13 — with GitHub Actions Inactive
@denik
denikforce-pushed the denik/acc-bundle-config branch from 31b42d0 to c4484b3CompareMay 13, 2025 19:20
@denik
denikforce-pushed the denik/acc-cleanup-bundle-section branch from 47c58e7 to 4cabc76CompareMay 13, 2025 19:22
@denik
deniktemporarily deployed to test-trigger-is May 13, 2025 19:23 — with GitHub Actions Inactive
@denik
deniktemporarily deployed to test-trigger-is May 13, 2025 19:43 — with GitHub Actions Inactive
@denik
denikforce-pushed the denik/acc-bundle-config branch from c4484b3 to bfa5241CompareMay 15, 2025 07:42
@denik
denikforce-pushed the denik/acc-cleanup-bundle-section branch from 901a212 to c5a0608CompareMay 15, 2025 07:43
@denik
deniktemporarily deployed to test-trigger-is May 15, 2025 07:43 — with GitHub Actions Inactive
@denik
denikforce-pushed the denik/acc-cleanup-bundle-section branch from c5a0608 to c2f20d2CompareMay 15, 2025 07:43
@denik
deniktemporarily deployed to test-trigger-is May 15, 2025 07:44 — with GitHub Actions Inactive
@denik
denikforce-pushed the denik/acc-bundle-config branch from bfa5241 to d3eb299CompareMay 15, 2025 07:46
@denik
denikforce-pushed the denik/acc-cleanup-bundle-section branch from c2f20d2 to bb4bc79CompareMay 15, 2025 07:46
@denik
deniktemporarily deployed to test-trigger-is May 15, 2025 07:46 — with GitHub Actions Inactive
@denik
denikforce-pushed the denik/acc-cleanup-bundle-section branch from bb4bc79 to 36811d6CompareMay 15, 2025 09:10
@denik
deniktemporarily deployed to test-trigger-is May 15, 2025 09:10 — with GitHub Actions Inactive
Base automatically changed from denik/acc-bundle-config to mainMay 19, 2025 13:21
github-merge-queueBot pushed a commit that referenced this pull request May 19, 2025
## Changes
New setting BundleConfig, to define portions of bundle config in
test.toml which is then propagated to all child configs (like any other
config setting).
## Why
This feature allows structuring tests better, by having common section
defined at the parent level and re-used by all children. This is also a
foundation for BundleConfigMatrix (similar to EnvMatrix) which will
allow to test variations on the config without duplicating test.
Some use cases:
- Today we have to copy bundle.name boilerplate in every test, this
becomes a one time thing at the parent level
(#2845)
- Today many artifact tests need to be test with both dynamic_version
and without, this would allow to reuse config definition. We also have
other variations for config, those can be matrix-tested with
BundleConfigMatrix.
- A number of tests use envsubst to parametrize. Using BundleConfig is a
replacement for that is declarative, inheritable and supports complex
structures where envsubst only works with strings.
## Tests
- New acceptance selftest.
- Testing in practice: #2845
@denik
denikforce-pushed the denik/acc-cleanup-bundle-section branch from 3914f22 to 0ccf171CompareMay 19, 2025 13:27
@denik
deniktemporarily deployed to test-trigger-is May 19, 2025 13:27 — with GitHub Actions Inactive
@denik

Copy link
Copy Markdown
ContributorAuthor

The configuration for the bind tests includes a UUID for the bundle name. This is not done for other tests. If they run with Cloud = true and run in parallel, they can interfere with each other. We can ensure they never interfere by including a UUID for all of them.

Have you observed this failure mode?

I haven't seen any failures, but I also don't see why not default to test-bundle-$UNIQUE_NAME everywhere, even if tests are local-only. That way we won't miss it if tests are updated to run from local to local+cloud.

See 0ccf171

@pietern

Copy link
Copy Markdown
Contributor

@denik I believe this only works when used in combination with .tmpl and envsubst, not out of the box.

You can see the output for some of these include $UNIQUE_ID instead of the replacement value [UNIQUE_ID].

@denik
denikforce-pushed the denik/acc-cleanup-bundle-section branch from 0ccf171 to 395b76bCompareMay 19, 2025 14:20
@denik

Copy link
Copy Markdown
ContributorAuthor

@denik I believe this only works when used in combination with .tmpl and envsubst, not out of the box.

You can see the output for some of these include $UNIQUE_ID instead of the replacement value [UNIQUE_ID].

Good catch, I dropped that commit. I got confused with Env / EnvMatrix which do process environment variables in the test runner.

We can implement something similar here (and thus drop the need for envsubst completely) but that would have to be a follow up.

@denik
deniktemporarily deployed to test-trigger-is May 19, 2025 14:22 — with GitHub Actions Inactive
@denik
denikforce-pushed the denik/acc-cleanup-bundle-section branch from 395b76b to 86dd830CompareMay 19, 2025 14:23
@denik
deniktemporarily deployed to test-trigger-is May 19, 2025 14:23 — with GitHub Actions Inactive
@denik
denik requested a review from pieternMay 20, 2025 12:42
@denik
denikforce-pushed the denik/acc-cleanup-bundle-section branch from 86dd830 to eb7f353CompareMay 21, 2025 09:30
@denik
deniktemporarily deployed to test-trigger-is May 21, 2025 09:30 — with GitHub Actions Inactive
@denik
deniktemporarily deployed to test-trigger-is May 21, 2025 09:34 — with GitHub Actions Inactive
@denik
deniktemporarily deployed to test-trigger-is May 21, 2025 09:38 — with GitHub Actions Inactive
@denik
denik enabled auto-merge May 21, 2025 09:39
@denik
denik added this pull request to the merge queueMay 21, 2025
Merged via the queue into main with commit 2333eb4May 21, 2025
@denik
denik deleted the denik/acc-cleanup-bundle-section branch May 21, 2025 10:11
denik added a commit that referenced this pull request May 20, 2026
## Changes
New setting BundleConfig, to define portions of bundle config in
test.toml which is then propagated to all child configs (like any other
config setting).
## Why
This feature allows structuring tests better, by having common section
defined at the parent level and re-used by all children. This is also a
foundation for BundleConfigMatrix (similar to EnvMatrix) which will
allow to test variations on the config without duplicating test.
Some use cases:
- Today we have to copy bundle.name boilerplate in every test, this
becomes a one time thing at the parent level
(#2845)
- Today many artifact tests need to be test with both dynamic_version
and without, this would allow to reuse config definition. We also have
other variations for config, those can be matrix-tested with
BundleConfigMatrix.
- A number of tests use envsubst to parametrize. Using BundleConfig is a
replacement for that is declarative, inheritable and supports complex
structures where envsubst only works with strings.
## Tests
- New acceptance selftest.
- Testing in practice: #2845
denik added a commit that referenced this pull request May 20, 2026
## Changes
Using BundleConfig option (#2809),
specify boilerplate bundle.name once and clean up individual test cases.
The tests with diagnostics (lineno, column) are left alone.
Test acceptance/bundle/volume_path is split into invidual tests, one
databricks.yml per test (since BundleConfig only updates
databricks.yml[.tmpl] in the root of the test.
## Why
Simpler to read and write, make each test focussed on what's important.
## Tests
Existing tests.
Benchmarked acceptance test suite (local) to ensure this does not add a
lot of overhead (due to parsing/marshalling every config):
```
This branch: Time (mean ± σ): 18.364 s ± 0.857 s [User: 32.707 s, System: 27.281 s]
Range (min … max): 17.002 s … 19.922 s 10 runs
Main:
Time (mean ± σ): 18.337 s ± 0.789 s [User: 32.613 s, System: 27.235 s]
Range (min … max): 17.078 s … 19.342 s 10 runs
```
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@denik@pietern