Fix integration tests

[dmcgowan]

Currently only a few tests are there, but running the integration tests is useful to show vm startup and init are working

[Copilot]

Pull request overview

This pull request fixes integration tests to demonstrate that VM startup and initialization are working correctly. The changes add a new test execution script and modify the test setup to properly handle PATH configuration and temporary directory resolution on different platforms (particularly macOS).

Changes:

• Added new shell script integration/test.sh to build and run integration tests individually
• Updated integration/main_test.go to improve PATH setup and handle symlink resolution for temp directories
• Added Makefile target test-integration to build test binary and execute tests using gotestsum
• Updated libkrun version from v1.15.1 to v1.17.4 and added required libcap-ng-dev dependency

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.

File	Description
integration/test.sh	New shell script to build test binary (with macOS code signing) and execute integration tests individually
integration/main_test.go	Enhanced PATH setup to include executable directory and improved temp directory handling with symlink resolution
Makefile	Added integration test build target and test-integration target using gotestsum
Dockerfile	Updated libkrun to v1.17.4 and added libcap-ng-dev dependency for the build

---

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

[Copilot]

There is a potential inconsistency between the Makefile and test.sh script. The Makefile builds the test binary with flags '${GO_LDFLAGS} ${GO_TAGS}', but the test.sh script builds it with just 'go test -c -o ../_output/integration.test .' without any flags. This means if the Makefile hasn't been run first, the test binary built by test.sh may have different build flags than expected, which could lead to inconsistent behavior.

Suggested change

	go test -c -o ../_output/integration.test .
	go test ${GO_LDFLAGS} ${GO_TAGS} -c -o ../_output/integration.test .

[Copilot]

There is a trailing blank line at the end of the file. While this is not an error, it's unnecessary and should be removed for cleaner code.

Suggested change

[Copilot]

The debug log statement using log.Printf with just "%v" to print the entire paths slice should be removed before production. This appears to be leftover debugging code that provides no value to users and clutters the output. If path information is needed for debugging, it should use a proper logging level or be removed entirely.

Suggested change

log.Printf("%v", paths)

[Copilot]

The slice prepending logic is inefficient. This creates a new element and shifts all existing elements for each path being prepended. A more efficient approach would be to build the new paths slice separately and then combine them, or use a simpler prepend pattern like: paths = append([]string{absPath}, paths...)

Suggested change

	paths = append(paths, "")
	copy(paths[1:], paths)
	paths[0] = absPath
	paths = append([]string{absPath}, paths...)

[Copilot]

Using t.Chdir can have side effects in tests that run concurrently. While the test.sh script runs tests with -test.parallel 1, if the tests are ever run directly with go test without this flag, the changing of the current working directory could affect other tests running in parallel. Consider adding t.Parallel() here to make it explicit that this test cannot be run in parallel, or use the working directory path directly without changing the global process state.

Suggested change

	t.Chdir(td)
	// Use Getwd to resolve symlinks (e.g., /var -> /private/var on macOS)
	resolvedTd, err := os.Getwd()
	if err != nil {
	t.Fatal("Failed to get current working directory:", err)
	// Resolve symlinks in the temporary directory path (e.g., /var -> /private/var on macOS)
	resolvedTd, err := filepath.EvalSymlinks(td)
	if err != nil {
	t.Fatal("Failed to resolve temporary directory path:", err)

[Copilot]

The shebang should use '#!/usr/bin/env bash' instead of '#!/bin/bash' for better portability across different systems where bash might be installed in different locations. The existing script in the codebase (script/install-proto-tools) uses '#!/usr/bin/env bash'.

Suggested change

	#!/bin/bash
	#!/usr/bin/env bash

[Copilot]

The shell script options 'set -e' are less comprehensive than the options used in the existing codebase. The script/install-proto-tools file uses 'set -eu -o pipefail', which also enables undefined variable errors (-u) and pipeline failures (-o pipefail). These options help catch more potential errors during script execution.

Suggested change

	set -e
	set -euo pipefail

[Copilot]

The test.sh script is missing the copyright header that is consistently present in other shell scripts in the codebase. For example, script/install-proto-tools includes the Apache License 2.0 copyright header. This should be added for consistency and proper licensing attribution.

Suggested change

	#!/bin/bash
	#!/bin/bash
	# Copyright 2024 The containerd Authors.
	#
	# Licensed under the Apache License, Version 2.0 (the "License");
	# you may not use this file except in compliance with the License.
	# You may obtain a copy of the License at
	#
	# http://www.apache.org/licenses/LICENSE-2.0
	#
	# Unless required by applicable law or agreed to in writing, software
	# distributed under the License is distributed on an "AS IS" BASIS,
	# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
	# See the License for the specific language governing permissions and
	# limitations under the License.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

Comments suppressed due to low confidence (1)

integration/main_test.go:81

• The error messages here use capitalized "Failed to..." which is inconsistent with the existing error messages in integration/vm_test.go that use lowercase "failed to...". For consistency within the integration package, consider using lowercase error messages.

				t.Fatal("Failed to get current working directory:", err)
			}
			vm, err := tc.vmm.NewInstance(t.Context(), resolvedTd)
			if err != nil {
				t.Fatal("Failed to create VM instance:", err)
			}

			if err := vm.Start(t.Context()); err != nil {
				t.Fatal("Failed to start VM instance:", err)

---

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

[Copilot]

The shell script builds the integration.test binary if it doesn't exist, but this duplicates the build logic from the Makefile and creates inconsistency. The Makefile target uses GO_LDFLAGS and GO_TAGS variables that won't be applied when building via this script. Additionally, since the test-integration Makefile target depends on _output/integration.test, this build check is redundant. The script should assume the binary exists (built by make) or fail with a clear error if it doesn't.

Suggested change

	# Build the test binary if it doesn't exist
	if [[ ! -f ../_output/integration.test ]]; then
	go test -c -o ../_output/integration.test .
	# Sign it with hypervisor entitlement on macOS
	if [[ "$OSTYPE" == "darwin"* ]]; then
	codesign --sign - --entitlements ../cmd/containerd-shim-nerdbox-v1/containerd-shim-nerdbox-v1.entitlements --force ../_output/integration.test &>/dev/null
	fi
	# Ensure the test binary exists (it should be built via the Makefile)
	if [[ ! -f ../_output/integration.test ]]; then
	echo "Error: ../_output/integration.test not found. Please build it first (e.g., via 'make test-integration')." >&2
	exit 1

[Copilot]

The codesign command arguments are in a different order compared to the Makefile. In test.sh it's "--sign - --entitlements ... --force" while in Makefile it's "--entitlements ... --force -s -". While both should work, this inconsistency makes maintenance harder. Consider using the same argument order as the Makefile for consistency.

Suggested change

	codesign --sign - --entitlements ../cmd/containerd-shim-nerdbox-v1/containerd-shim-nerdbox-v1.entitlements --force ../_output/integration.test &>/dev/null
	codesign --entitlements ../cmd/containerd-shim-nerdbox-v1/containerd-shim-nerdbox-v1.entitlements --force -s - ../_output/integration.test &>/dev/null

[Copilot]

The prepending logic in this loop is inefficient and has confusing behavior. Each iteration appends an empty string to paths, copies all elements forward, and sets the first element. This results in paths being prepended in reverse order - "." will be added first (index 0), then "../_output" will be added at index 0, pushing "." to index 1. This means the final PATH will have "../_output" before "." which may not be the intended order based on the slice definition. Consider using a simpler approach like reversing the iteration order or building a new slice with explicit ordering.

Suggested change

	for _, p := range []string{
	"../_output",
	".",
	} {
	absPath := filepath.Clean(filepath.Join(exeDir, p))
	// Prepend to slice
	paths = append(paths, "")
	copy(paths[1:], paths)
	paths[0] = absPath
	}
	var prependPaths []string
	for _, p := range []string{
	"../_output",
	".",
	} {
	absPath := filepath.Clean(filepath.Join(exeDir, p))
	prependPaths = append(prependPaths, absPath)
	}
	paths = append(prependPaths, paths...)

[Copilot]

This debug log statement should be removed or placed behind a debug flag before merging. It will clutter test output with the full PATH environment variable on every test run.

Suggested change

	log.Printf("%v", paths)
	if testing.Verbose() {
	log.Printf("%v", paths)
	}

[Copilot]

The test-integration target uses 'gotestsum' which is not included in the repository's dependencies or build setup. This tool needs to be either added as a dependency (e.g., via go install in a setup step) or documented as a prerequisite in the README, or the Makefile should check for its existence and provide a helpful error message if it's not installed.

[Copilot]

The libkrun dependency is fetched via git clone --branch ${LIBKRUN_VERSION} using a mutable Git tag (ARG LIBKRUN_VERSION=v1.17.4), which means a compromised or retagged upstream repository could silently deliver malicious code into your build process. Because this clone happens during the image build and the resulting library is used at runtime, an attacker controlling the tag or repository could inject arbitrary code into production artifacts. To mitigate this supply-chain risk, pin libkrun to an immutable reference (e.g., a specific commit SHA) and optionally verify its integrity (e.g., via checksum or signature) before building.