Skip to content

Conversation

@jsternberg
Copy link
Collaborator

This adds integration tests for the dap build command to test various
behavior associated with the command. We start the build and the
integration test acts as a dap client to send requests and check that
the output is what we expect.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request adds comprehensive integration tests for the dap build command, which implements a Debug Adapter Protocol (DAP) server for debugging Dockerfile builds. The PR refactors the DAP test utilities into a reusable package and creates tests that exercise various debugging features including breakpoints, stepping, and stack inspection.

Key changes:

  • Extracted DAP test utilities (Client and connection logging) into a dedicated util/daptest package for reusability across integration tests
  • Created comprehensive integration tests covering DAP debugging scenarios: basic build, stop-on-entry, breakpoints, step-in/next/out operations
  • Refactored common types into dap/common package to avoid circular dependencies between production and test code

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file
File Description
util/daptest/conn.go New logging connection wrapper for DAP messages in tests with output buffering for event handling
util/daptest/client.go Moved and enhanced DAP test client from dap package, supporting multiple event handlers per event type
tests/dap_build.go New integration tests for dap build command covering all major debugging operations
tests/integration_test.go Registered new dap build tests in the integration test suite
dap/common/types.go Extracted common Conn interface to break circular dependencies
dap/conn.go Updated to use type alias referencing common.Conn
dap/client.go Emptied file (code moved to util/daptest)
dap/adapter_test.go Updated to use utilities from util/daptest package
dap/adapter.go Minor comment formatting improvement (capitalization)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@jsternberg jsternberg force-pushed the dap-build-integration-tests branch from c78b2b1 to 49d5faa Compare December 11, 2025 22:21
@jsternberg jsternberg marked this pull request as draft December 11, 2025 22:56
@jsternberg jsternberg force-pushed the dap-build-integration-tests branch 3 times, most recently from b43f52c to 4b52104 Compare December 12, 2025 18:20
@jsternberg jsternberg force-pushed the dap-build-integration-tests branch from 4b52104 to 7191c16 Compare December 12, 2025 18:28
@jsternberg jsternberg marked this pull request as ready for review December 12, 2025 19:34
@jsternberg jsternberg force-pushed the dap-build-integration-tests branch from 7191c16 to fe3d6b0 Compare December 15, 2025 19:01
Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

Looks like the new tests are not running in CI atm

require.NoError(t, done(false))
}

func testDapBuildExec(t *testing.T, sb integration.Sandbox) {
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to merge in the stubs

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed.

return e
}

type ExpectedStackFrame struct {
Copy link
Member

Choose a reason for hiding this comment

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

nit: I guess this doesn't need to be public

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Renamed to stackFrameMatcher and renamed some of the methods since Expected is kind of an awkward name.

@jsternberg jsternberg force-pushed the dap-build-integration-tests branch from fe3d6b0 to 4393f73 Compare December 16, 2025 16:28
@jsternberg
Copy link
Collaborator Author

@tonistiigi the tests only run on the experimental matrix builds because they don't presently work on the non-experimental branch because dap only works with experimental enabled.

@jsternberg jsternberg force-pushed the dap-build-integration-tests branch from 4393f73 to 65b61df Compare December 16, 2025 16:44
This adds integration tests for the `dap build` command to test various
behavior associated with the command. We start the build and the
integration test acts as a dap client to send requests and check that
the output is what we expect.

Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
@jsternberg jsternberg force-pushed the dap-build-integration-tests branch from 65b61df to fdfba30 Compare December 17, 2025 16:56
@jsternberg
Copy link
Collaborator Author

Including additional unit tests for checking the variable functionality.

@tonistiigi tonistiigi merged commit 2bcb098 into docker:master Dec 17, 2025
138 checks passed
@jsternberg jsternberg deleted the dap-build-integration-tests branch December 17, 2025 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants