Skip to content

Add support for recording requests in integration acceptance tests #2720

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 68 commits into from
May 8, 2025

Conversation

shreyas-goenka
Copy link
Contributor

@shreyas-goenka shreyas-goenka commented Apr 14, 2025

Changes

This PR adds support for recording API calls made during an integration acceptance test by setting RecordRequests = true in the test.toml file.

This PR adds a new struct ProxyServer which is a server that sits in between a client and a real Databricks workspace. Having a server in between allows us to record requests as they are sent to a real Databricks workspace in our integration tests.

Why

Other than being a useful feature in itself, this also unlocks migration away from Terraform in DABs since we can now record the API requests made by Terraform and ensure that our custom library makes the same API requests in all cases.

Tests

A bunch of new acceptance tests for the new feature.

@shreyas-goenka shreyas-goenka changed the base branch from main to refactor-server April 14, 2025 22:30
Base automatically changed from refactor-server to main April 21, 2025 19:51
@shreyas-goenka shreyas-goenka requested a review from denik May 7, 2025 11:02
@shreyas-goenka shreyas-goenka requested a review from denik May 7, 2025 14:24
@shreyas-goenka shreyas-goenka enabled auto-merge May 8, 2025 08:04
@shreyas-goenka shreyas-goenka added this pull request to the merge queue May 8, 2025
Merged via the queue into main with commit 52c2f92 May 8, 2025
10 checks passed
@shreyas-goenka shreyas-goenka deleted the proxy-integration branch May 8, 2025 08:19
# hello, world\n base64 encoded in windows
[[Repls]]
Old = "aGVsbG8sIHdvcmxkDQo="
New = "[HELLO-WORLD]"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does this nuance show up?

I see the comment about newlines in the .wsignore file, but shouldn't the output always be verbatim?

Copy link
Contributor Author

@shreyas-goenka shreyas-goenka May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but shouldn't the output always be verbatim?

The difference I believe is \nvs \r\n that's being printed on the console. I did not actually decode and inspect it byte by byte though. Running the base64 deserializer through both returns "hello, world(newline)"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get that. What I wonder is, where does the \n or \r\n come from, if the fs cat command outputs file contents verbatim, and the input file doesn't have a newline.

Copy link
Contributor Author

@shreyas-goenka shreyas-goenka May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The input file here does have a newline. The one in volume-io file test does not, because the new line was hard to normalize in the API request payload.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's worth making this behave in a single way instead of accounting for the differences in the test itself.

We're likely seeing this: https://docs.github.com/en/get-started/git-basics/configuring-git-to-handle-line-endings

I suspect we can fix this by configuring everything under acceptance to use eol=lf.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, trying to address that in #2865.

github-merge-queue bot pushed a commit that referenced this pull request May 19, 2025
## Why
Based on feedback in
#2720 (comment). We
should always treat new lines in acceptance tests in windows as `\n`.
This is important because the characters itself can be semantically
important in tests. For example, an additional `\r` character in windows
changes the upload payload if you are uploading the file's content to a
workspace.

## Tests
Existing workspace-io test should no longer need a custom override for
windows.
Sign up for free to 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.

3 participants