diff --git a/.github/skills/azd-code-reviewer/reviewers.md b/.github/skills/azd-code-reviewer/reviewers.md index 51083488107..dab622fe7cb 100644 --- a/.github/skills/azd-code-reviewer/reviewers.md +++ b/.github/skills/azd-code-reviewer/reviewers.md @@ -199,6 +199,8 @@ Focus areas: - Test helpers: are setup/teardown patterns consistent? Are test utilities reused? - Integration vs unit: is the test level appropriate for what is being tested? - Flakiness risk: time-dependent tests, order-dependent tests, external service calls without mocks +- State isolation: do functional tests that modify user-level state use a dedicated `AZD_CONFIG_DIR` rather than shared or process-wide state? +- Fixture safety: are type assertions on values read from maps, JSON, environment files, or recordings guarded with `require` (presence + type) so malformed data fails the test instead of panicking and aborting the package? Review tests statically — do not attempt to run them. @@ -227,6 +229,7 @@ Focus areas: - Help text: is `--help` complete, with examples? - Interactive prompts: are they skippable with flags for CI/automation? - Progress feedback: for long operations, is there progress indication? +- Writer propagation: do nested formatters, spinners, prompts, and cursors use the injected writer without leaking terminal output to global stdout? - Exit codes: are they meaningful and documented? - Consistency: does this match the patterns in existing azd commands? diff --git a/cli/azd/AGENTS.md b/cli/azd/AGENTS.md index 1a1efc76ab0..c738d477c52 100644 --- a/cli/azd/AGENTS.md +++ b/cli/azd/AGENTS.md @@ -359,7 +359,10 @@ This project uses Go 1.26. Use modern standard library features: ### Testing Best Practices - **Test the actual rules, not just the framework**: When adding YAML-based error suggestion rules, write tests that exercise the rules end-to-end through the pipeline, not just tests that validate the framework's generic matching behavior -- **Extract shared test helpers**: Don't duplicate test utilities across files. Extract common helpers (e.g., shell wrappers, auth token fetchers, CLI runners) into shared `test_utils` packages. Duplication across 3+ files should always be refactored +- **Search for prior art before adding helpers**: Search `test/`, `pkg/osutil`, and the package under test before adding cleanup, retry, process, or platform-specific helpers. Reuse or promote existing behavior and extract shared test utilities instead of creating parallel implementations +- **Isolate persistent user state**: Functional tests that install extensions or mutate config, auth, cache, or other user-level state must set `AZD_CONFIG_DIR` to a dedicated `tempDirWithDiagnostics(t)` directory. Never let parallel tests read or modify the developer/agent's default `~/.azd` state +- **Make cleanup process-safe**: Wait for spawned commands to finish, register cleanup when state is created, and report failures. Use `osutil.RemoveAll` for directories containing executed binaries because Windows may retain transient locks; keep retries at the filesystem boundary instead of adding test sleeps. Since `t.Context()` is canceled before cleanup runs, use a separate bounded context when cleanup needs one +- **Guard fixture type assertions**: Check presence and type with `require` before casting values loaded from maps, JSON, environment files, or recordings. A raw type assertion panic can abort the package and hide the initiating failure - **Use correct env vars for testing**: - Non-interactive mode: `AZD_FORCE_TTY=false` (not `AZD_DEBUG_FORCE_NO_TTY`, which doesn't exist) - No-prompt mode: use the `--no-prompt` flag for core azd commands; `AZD_NO_PROMPT=true` is only used for propagating no-prompt into extension subprocesses @@ -370,7 +373,7 @@ This project uses Go 1.26. Use modern standard library features: - **Cross-platform paths**: When resolving binary paths in tests, handle `.exe` suffix on Windows (e.g., `azd` vs `azd.exe` via `process.platform === "win32"`) - **Test new JSON fields**: When adding fields to JSON command output (e.g., `expiresOn` in `azd auth status --output json`), add a test asserting the field's presence and format - **No unused dependencies**: Don't add npm/pip packages that aren't imported anywhere. Remove dead `devDependencies` before submitting -- **Never write to `os.Stdout` in tests**: Tests that write directly to `os.Stdout` (via `fmt.Print*`, `os.Stdout`, or UX components like `ux.NewSpinner` without a `Writer` option) corrupt the `go test -json` event stream under parallel execution, causing phantom test failures in CI. Use `t.Log`/`t.Logf` for diagnostic output, `io.Discard` or `&bytes.Buffer{}` for UX component writers, and `SkipLoadingSpinner: true` for prompt tests that don't need spinner behavior. Enforced by the `forbidigo` linter in `.golangci.yaml`. See [#8385](https://github.com/Azure/azure-dev/issues/8385) for details. +- **Keep terminal output on the injected writer**: Commands and UX components must pass the caller's `io.Writer` to every nested spinner, prompt, cursor, and formatter. In tests, use `t.Log`/`t.Logf` for diagnostics and `io.Discard` or `&bytes.Buffer{}` for UX writers. Raw stdout corrupts `go test -json` and creates phantom CI failures. `forbidigo` blocks direct writes in tests, but production writer propagation still needs buffer-based coverage. See [#8385](https://github.com/Azure/azure-dev/issues/8385) ## MCP Tools diff --git a/cli/azd/cmd/extension.go b/cli/azd/cmd/extension.go index 820485fb4fd..b388933bda9 100644 --- a/cli/azd/cmd/extension.go +++ b/cli/azd/cmd/extension.go @@ -1825,7 +1825,7 @@ func (a *extensionUninstallAction) Run(ctx context.Context) (*actions.ActionResu stepMessage += fmt.Sprintf(" (%s)", installed.Version) a.console.ShowSpinner(ctx, stepMessage, input.Step) - if err := a.extensionManager.Uninstall(extensionId); err != nil { + if err := a.extensionManager.Uninstall(ctx, extensionId); err != nil { a.console.StopSpinner(ctx, stepMessage, input.StepFailed) return nil, fmt.Errorf("failed to uninstall extension: %w", err) } diff --git a/cli/azd/cmd/tool.go b/cli/azd/cmd/tool.go index 4a21662a01f..ac668528fdb 100644 --- a/cli/azd/cmd/tool.go +++ b/cli/azd/cmd/tool.go @@ -183,15 +183,18 @@ func toolActions(root *actions.ActionDescriptor) *actions.ActionDescriptor { type toolAction struct { manager *tool.Manager console input.Console + writer io.Writer } func newToolAction( manager *tool.Manager, console input.Console, + writer io.Writer, ) actions.Action { return &toolAction{ manager: manager, console: console, + writer: writer, } } @@ -206,6 +209,7 @@ func (a *toolAction) Run(ctx context.Context) (*actions.ActionResult, error) { spinner := uxlib.NewSpinner(&uxlib.SpinnerOptions{ Text: "Detecting tools...", ClearOnStop: true, + Writer: a.writer, }) if err := spinner.Run(ctx, func(ctx context.Context) error { var detectErr error @@ -374,6 +378,7 @@ func (a *toolListAction) Run(ctx context.Context) (*actions.ActionResult, error) spinner := uxlib.NewSpinner(&uxlib.SpinnerOptions{ Text: "Checking tool status...", ClearOnStop: true, + Writer: a.writer, }) if err := spinner.Run(ctx, func(ctx context.Context) error { var detectErr error @@ -897,6 +902,7 @@ func detectAllTools( ctx context.Context, manager *tool.Manager, formatter output.Formatter, + writer io.Writer, spinnerText string, ) ([]*tool.ToolStatus, error) { if formatter != nil && formatter.Kind() == output.JsonFormat { @@ -907,6 +913,7 @@ func detectAllTools( spinner := uxlib.NewSpinner(&uxlib.SpinnerOptions{ Text: spinnerText, ClearOnStop: true, + Writer: writer, }) if err := spinner.Run(ctx, func(ctx context.Context) error { var detectErr error @@ -1080,7 +1087,7 @@ func (a *toolInstallAction) resolveToolIds(ctx context.Context) ([]string, error // --all: install all recommended tools that are not already installed. if a.flags.all { - statuses, err := detectAllTools(ctx, a.manager, a.formatter, "Detecting tool status...") + statuses, err := detectAllTools(ctx, a.manager, a.formatter, a.writer, "Detecting tool status...") if err != nil { return nil, fmt.Errorf("detecting tools: %w", err) } @@ -1100,7 +1107,7 @@ func (a *toolInstallAction) resolveToolIds(ctx context.Context) ([]string, error } // Interactive: let the user pick from uninstalled tools. - statuses, err := detectAllTools(ctx, a.manager, a.formatter, "Detecting tool status...") + statuses, err := detectAllTools(ctx, a.manager, a.formatter, a.writer, "Detecting tool status...") if err != nil { return nil, fmt.Errorf("detecting tools: %w", err) } @@ -1439,7 +1446,7 @@ func (a *toolUpgradeAction) Run(ctx context.Context) (*actions.ActionResult, err // detectInstalledTools runs DetectAll behind a spinner and returns the full // set of tool statuses. Used by the --all and interactive upgrade paths. func (a *toolUpgradeAction) detectInstalledTools(ctx context.Context) ([]*tool.ToolStatus, error) { - statuses, err := detectAllTools(ctx, a.manager, a.formatter, "Detecting installed tools...") + statuses, err := detectAllTools(ctx, a.manager, a.formatter, a.writer, "Detecting installed tools...") if err != nil { return nil, fmt.Errorf("detecting installed tools: %w", err) } @@ -1757,7 +1764,7 @@ func (a *toolUninstallAction) resolveToolIds(ctx context.Context) ([]string, err // --all, --dry-run, and the interactive picker all need the current // installed set. - statuses, err := detectAllTools(ctx, a.manager, a.formatter, "Detecting installed tools...") + statuses, err := detectAllTools(ctx, a.manager, a.formatter, a.writer, "Detecting installed tools...") if err != nil { return nil, fmt.Errorf("detecting installed tools: %w", err) } @@ -1920,6 +1927,7 @@ func (a *toolCheckAction) Run(ctx context.Context) (*actions.ActionResult, error spinner := uxlib.NewSpinner(&uxlib.SpinnerOptions{ Text: "Checking for upgrades...", ClearOnStop: true, + Writer: a.writer, }) if err := spinner.Run(ctx, func(ctx context.Context) error { var detectErr error @@ -2118,6 +2126,7 @@ func (a *toolShowAction) Run(ctx context.Context) (*actions.ActionResult, error) spinner := uxlib.NewSpinner(&uxlib.SpinnerOptions{ Text: fmt.Sprintf("Checking %s...", toolDef.Name), ClearOnStop: true, + Writer: a.writer, }) if err := spinner.Run(ctx, func(ctx context.Context) error { var detectErr error diff --git a/cli/azd/cmd/tool_test.go b/cli/azd/cmd/tool_test.go index 2b619e69b0b..fd579b40f12 100644 --- a/cli/azd/cmd/tool_test.go +++ b/cli/azd/cmd/tool_test.go @@ -25,6 +25,13 @@ import ( "github.com/stretchr/testify/require" ) +// ANSI cursor-visibility control sequences a spinner emits to the writer: +// it hides the cursor while running and shows it again when it stops. +const ( + ansiHideCursor = "\033[?25l" + ansiShowCursor = "\033[?25h" +) + func TestToolCommandGating(t *testing.T) { // The "tool" command group is always registered, regardless of any // alpha feature gating. @@ -330,6 +337,106 @@ func TestToolShowAction_InvalidArgDoesNotEmitToolId(t *testing.T) { "tool.id must not be emitted when FindTool fails on an unknown tool") } +func TestToolActionSpinnersUseInjectedWriter(t *testing.T) { + detectErr := errors.New("detect failed") + + tests := []struct { + name string + run func(context.Context, *bytes.Buffer) error + }{ + { + name: "bare tool action", + run: func(ctx context.Context, writer *bytes.Buffer) error { + detector := &cmdMockDetector{ + detectAll: func(context.Context, []*tool.ToolDefinition) ([]*tool.ToolStatus, error) { + return nil, detectErr + }, + } + action := newToolAction( + tool.NewManager(detector, &cmdMockInstaller{}, nil), + mockinput.NewMockConsole(), + writer, + ) + _, err := action.Run(ctx) + return err + }, + }, + { + name: "tool list", + run: func(ctx context.Context, writer *bytes.Buffer) error { + detector := &cmdMockDetector{ + detectAll: func(context.Context, []*tool.ToolDefinition) ([]*tool.ToolStatus, error) { + return nil, detectErr + }, + } + action := newToolListAction( + tool.NewManager(detector, &cmdMockInstaller{}, nil), + mockinput.NewMockConsole(), + &output.TableFormatter{}, + writer, + ) + _, err := action.Run(ctx) + return err + }, + }, + { + name: "tool check", + run: func(ctx context.Context, writer *bytes.Buffer) error { + detector := &cmdMockDetector{ + detectAll: func(context.Context, []*tool.ToolDefinition) ([]*tool.ToolStatus, error) { + return nil, detectErr + }, + } + updateChecker := tool.NewUpdateChecker( + &memUserConfigManager{}, + detector, + func() (string, error) { return t.TempDir(), nil }, + nil, + ) + action := newToolCheckAction( + tool.NewManager(detector, &cmdMockInstaller{}, updateChecker), + mockinput.NewMockConsole(), + &output.TableFormatter{}, + writer, + ) + _, err := action.Run(ctx) + return err + }, + }, + { + name: "tool show", + run: func(ctx context.Context, writer *bytes.Buffer) error { + detector := &cmdMockDetector{ + detectTool: func(context.Context, *tool.ToolDefinition) (*tool.ToolStatus, error) { + return nil, detectErr + }, + } + manager := tool.NewManager(detector, &cmdMockInstaller{}, nil) + action := newToolShowAction( + []string{manager.GetAllTools()[0].Id}, + manager, + mockinput.NewMockConsole(), + &output.NoneFormatter{}, + writer, + ) + _, err := action.Run(ctx) + return err + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + var writer bytes.Buffer + err := test.run(t.Context(), &writer) + + require.ErrorIs(t, err, detectErr) + assert.Contains(t, writer.String(), ansiHideCursor) + assert.Contains(t, writer.String(), ansiShowCursor) + }) + } +} + // lookupToolBoolUsage returns the most recent bool-valued usage attribute // for the given key (assumes the key was set via .Bool()). func lookupToolBoolUsage(key string) (bool, bool) { @@ -1275,13 +1382,14 @@ func TestToolUpgradeAction_All_UpgradesInstalledTools(t *testing.T) { } manager := tool.NewManager(detector, installer, nil) + var writer bytes.Buffer action := newToolUpgradeAction( nil, &toolUpgradeFlags{all: true}, manager, mockinput.NewMockConsole(), &output.NoneFormatter{}, - io.Discard, + &writer, ) _, err := action.Run(t.Context()) @@ -1290,6 +1398,8 @@ func TestToolUpgradeAction_All_UpgradesInstalledTools(t *testing.T) { require.Len(t, installedIDs, 2) assert.ElementsMatch(t, installedIDs, upgradedIDs, "--all must upgrade exactly the installed tools") + assert.Contains(t, writer.String(), "\033[?25l") + assert.Contains(t, writer.String(), "\033[?25h") } // TestToolUpgradeAction_All_JsonFormat_EmitsCleanJson exercises the reviewer's diff --git a/cli/azd/docs/recording-functional-tests-guide.md b/cli/azd/docs/recording-functional-tests-guide.md index 69cef6d94fa..6d63c77acfb 100644 --- a/cli/azd/docs/recording-functional-tests-guide.md +++ b/cli/azd/docs/recording-functional-tests-guide.md @@ -718,7 +718,27 @@ if session == nil { } ``` -### 7. Add Debug Logging +### 7. Isolate State and Use Shared Cleanup + +Use a private `AZD_CONFIG_DIR` when a test changes user-level state so parallel tests do not share `~/.azd`. Set it on the test CLI before the first `azd` invocation rather than using `t.Setenv`, which changes process-wide state: + +```go +configDir := tempDirWithDiagnostics(t) +cli.Env = append(cli.Env, "AZD_CONFIG_DIR="+configDir) +``` + +A new config directory is logged out, so tests that need live Azure access must authenticate in it. Leave `AZD_CONFIG_DIR` unset when a test only uses the existing login. + +Use `tempDirWithDiagnostics(t)` for generated or executed files. It retries transient Windows file locks and reports lock diagnostics if cleanup fails. Register other cleanup when state is created; don't duplicate retry loops or add fixed sleeps. Since `t.Context()` is canceled before cleanup runs, use a separate bounded context when needed. + +Guard type assertions when reading untyped fixture, JSON, environment, or recording data so malformed input fails the test instead of panicking: + +```go +subscriptionID, ok := envValues["AZURE_SUBSCRIPTION_ID"].(string) +require.True(t, ok, "AZURE_SUBSCRIPTION_ID should be a string") +``` + +### 8. Add Debug Logging ```go t.Logf("Recording mode: playback=%v", session != nil && session.Playback) @@ -726,7 +746,7 @@ t.Logf("Environment name: %s", envName) t.Logf("Subscription ID: %s", subscriptionId) ``` -### 8. Handle Skipped Tests Gracefully +### 9. Handle Skipped Tests Gracefully **DO**: ```go @@ -1022,10 +1042,12 @@ os.Setenv("AZD_TEST_FIXED_CLOCK_UNIX_TIME", "1744738873") - [ ] Start with `recording.Start(t)` - [ ] Use `randomOrStoredEnvName(session)` - [ ] Create CLI with `azdcli.WithSession(session)` +- [ ] Use a private `AZD_CONFIG_DIR` for user-level state changes; authenticate it if live Azure access is required - [ ] Use `session.ProxyClient` for HTTP operations - [ ] Store dynamic values in `session.Variables` -- [ ] Add cleanup with session checks +- [ ] Use `tempDirWithDiagnostics(t)` and register other cleanup immediately - [ ] Use appropriate timeouts for recording/playback +- [ ] Guard fixture type assertions so malformed data fails the test without panicking ### Recording a Test ✓ - [ ] Set `AZURE_RECORD_MODE=record` @@ -1056,8 +1078,3 @@ os.Setenv("AZD_TEST_FIXED_CLOCK_UNIX_TIME", "1744738873") - **Test Helpers**: `cli/azd/test/azdcli/` - **Example Tests**: `cli/azd/test/functional/up_test.go` - **go-vcr Documentation**: https://github.com/dnaeon/go-vcr - ---- - -**Last Updated**: December 2025 -**Maintainers**: Azure Developer CLI Team diff --git a/cli/azd/pkg/extensions/manager.go b/cli/azd/pkg/extensions/manager.go index ec9fe164b50..65fae091afc 100644 --- a/cli/azd/pkg/extensions/manager.go +++ b/cli/azd/pkg/extensions/manager.go @@ -781,8 +781,8 @@ func (m *Manager) installInternal( return selectedVersion, nil } -// Uninstall an extension by name -func (m *Manager) Uninstall(id string) error { +// Uninstall uninstalls an extension by name. +func (m *Manager) Uninstall(ctx context.Context, id string) error { // Get the installed extension extension, err := m.GetInstalled(FilterOptions{Id: id}) if err != nil { @@ -795,16 +795,8 @@ func (m *Manager) Uninstall(id string) error { } extensionDir := filepath.Join(userConfigDir, "extensions", extension.Id) - if err := os.MkdirAll(extensionDir, os.ModePerm); err != nil { - return fmt.Errorf("failed to create target directory: %w", err) - } - - // Remove the extension artifacts when it exists - _, err = os.Stat(extensionDir) - if err == nil { - if err := os.RemoveAll(extensionDir); err != nil { - return fmt.Errorf("failed to remove extension: %w", err) - } + if err := osutil.RemoveAll(ctx, extensionDir); err != nil { + return fmt.Errorf("failed to remove extension: %w", err) } // Update the user config @@ -901,7 +893,7 @@ func (m *Manager) upgradeInternal( opts UpgradeOptions, visited map[string]struct{}, ) (*ExtensionVersion, []UpgradeResult, error) { - if err := m.Uninstall(extension.Id); err != nil { + if err := m.Uninstall(ctx, extension.Id); err != nil { return nil, nil, fmt.Errorf("failed to uninstall extension: %w", err) } diff --git a/cli/azd/pkg/extensions/manager_test.go b/cli/azd/pkg/extensions/manager_test.go index 6ae6724a16f..8ea9d337809 100644 --- a/cli/azd/pkg/extensions/manager_test.go +++ b/cli/azd/pkg/extensions/manager_test.go @@ -188,7 +188,7 @@ func Test_List_Install_Uninstall_Flow(t *testing.T) { require.Greater(t, len(installed), 0) // Uninstall the first extension - err = manager.Uninstall(extensions[0].Id) + err = manager.Uninstall(t.Context(), extensions[0].Id) require.NoError(t, err) // List installed extensions (expect 0) @@ -320,7 +320,7 @@ func Test_Install_With_SemverConstraints(t *testing.T) { require.NotNil(t, extensionVersion) require.Equal(t, tc.Expected, extensionVersion.Version) - err = manager.Uninstall("test.extension") + err = manager.Uninstall(t.Context(), "test.extension") require.NoError(t, err) } }) @@ -2334,7 +2334,7 @@ func Test_Upgrade_DependencyUpgrade_RefusesToDowngradeOutsideConstraint(t *testi require.NoError(t, err) // Simulate the user upgrading the dependency standalone to a version outside the constraint. - require.NoError(t, manager.Uninstall("test.child")) + require.NoError(t, manager.Uninstall(t.Context(), "test.child")) children, err := manager.FindExtensions(*mockContext.Context, &FilterOptions{Id: "test.child"}) require.NoError(t, err) _, err = manager.Install(*mockContext.Context, children[0], "2.0.0") @@ -2560,7 +2560,7 @@ func Test_Upgrade_DependencyUpgrade_EmptyConstraint(t *testing.T) { // Force the child to an older version to confirm dependency upgrade does NOT // run when the parent's dependency constraint is empty. - _ = manager.Uninstall("test.child") + _ = manager.Uninstall(t.Context(), "test.child") childMeta, err := manager.FindExtensions(*mockContext.Context, &FilterOptions{Id: "test.child"}) require.NoError(t, err) _, err = manager.Install(*mockContext.Context, childMeta[0], "1.0.0") diff --git a/cli/azd/pkg/osutil/rename_unix.go b/cli/azd/pkg/osutil/rename_unix.go index baf9e9492c7..301c86c43bf 100644 --- a/cli/azd/pkg/osutil/rename_unix.go +++ b/cli/azd/pkg/osutil/rename_unix.go @@ -14,3 +14,9 @@ import ( func Rename(ctx context.Context, old, new string) error { return os.Rename(old, new) } + +// RemoveAll removes path and any children it contains. The context is retained +// for API parity with Windows, where it cancels retry backoff. +func RemoveAll(_ context.Context, path string) error { + return os.RemoveAll(path) +} diff --git a/cli/azd/pkg/osutil/rename_windows.go b/cli/azd/pkg/osutil/rename_windows.go index ead42ff2781..d9a492fa27c 100644 --- a/cli/azd/pkg/osutil/rename_windows.go +++ b/cli/azd/pkg/osutil/rename_windows.go @@ -8,6 +8,7 @@ package osutil import ( "context" "errors" + "fmt" "log" "os" "time" @@ -20,15 +21,24 @@ import ( // Rename fails due to what may be transient file system errors. This can help work around issues where the file may // temporary be opened by a virus scanner or some other process which prevents us from renaming the file. func Rename(ctx context.Context, old, new string) error { - return retry.Do(ctx, retry.WithMaxRetries(10, retry.NewConstant(1*time.Second)), func(ctx context.Context) error { - err := os.Rename(old, new) - if errors.Is(err, windows.ERROR_SHARING_VIOLATION) { - // If some other process has a open handle to the source file, Rename can fail with ERROR_SHARING_VIOLATION. - log.Printf("rename of %s to %s failed due to ERROR_SHARING_VIOLATION, allowing retry", old, new) - return retry.RetryableError(err) - } else if errors.Is(err, windows.ERROR_ACCESS_DENIED) { - // If the target file has already exists and is in use, Rename can fail with ERROR_ACCESS_DENIED. - log.Printf("rename of %s to %s failed due to ERROR_ACCESS_DENIED, allowing retry", old, new) + return retryFileSystemOperation(ctx, fmt.Sprintf("rename of %s to %s", old, new), func() error { + return os.Rename(old, new) + }) +} + +// RemoveAll removes path and any children it contains, retrying transient Windows file locks. +func RemoveAll(ctx context.Context, path string) error { + return retryFileSystemOperation(ctx, fmt.Sprintf("remove of %s", path), func() error { + return os.RemoveAll(path) + }) +} + +func retryFileSystemOperation(ctx context.Context, description string, operation func() error) error { + return retry.Do(ctx, retry.WithMaxRetries(10, retry.NewConstant(time.Second)), func(context.Context) error { + err := operation() + if errors.Is(err, windows.ERROR_SHARING_VIOLATION) || + errors.Is(err, windows.ERROR_ACCESS_DENIED) { + log.Printf("%s failed due to a transient file lock, allowing retry", description) return retry.RetryableError(err) } return err diff --git a/cli/azd/pkg/osutil/rename_windows_test.go b/cli/azd/pkg/osutil/rename_windows_test.go index b8644aa6309..dfc05f9d678 100644 --- a/cli/azd/pkg/osutil/rename_windows_test.go +++ b/cli/azd/pkg/osutil/rename_windows_test.go @@ -6,12 +6,15 @@ package osutil import ( + "context" "os" "path/filepath" "testing" "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/sys/windows" ) func TestRename(t *testing.T) { @@ -19,7 +22,7 @@ func TestRename(t *testing.T) { dir := t.TempDir() file, err := os.Create(filepath.Join(dir, "old")) - assert.NoError(t, err) + require.NoError(t, err) // Wait for a moment before closing the file. This allows Rename to exercise the retry logic for a sharing violation // since while we hold the file open, os.Rename will fail. @@ -38,12 +41,12 @@ func TestRename(t *testing.T) { t.Run("Destination In Use", func(t *testing.T) { dir := t.TempDir() file, err := os.Create(filepath.Join(dir, "old")) - assert.NoError(t, err) + require.NoError(t, err) err = file.Close() - assert.NoError(t, err) + require.NoError(t, err) file, err = os.Create(filepath.Join(dir, "new")) - assert.NoError(t, err) + require.NoError(t, err) // Wait for a moment before closing the target. This allows Rename to exercise the retry logic for an access is // denied error since we hold the file open, os.Rename will fail. @@ -61,3 +64,31 @@ func TestRename(t *testing.T) { assert.NoError(t, err) }) } + +func TestRemoveAll_FileInUse(t *testing.T) { + dir := t.TempDir() + file, err := os.Create(filepath.Join(dir, "locked")) + require.NoError(t, err) + + go func() { + time.Sleep(time.Second) + _ = file.Close() + }() + + assert.NoError(t, RemoveAll(t.Context(), dir)) + assert.NoDirExists(t, dir) +} + +func TestRetryFileSystemOperation_ContextCancellation(t *testing.T) { + ctx, cancel := context.WithCancel(t.Context()) + attempts := 0 + + err := retryFileSystemOperation(ctx, "test operation", func() error { + attempts++ + cancel() + return windows.ERROR_SHARING_VIOLATION + }) + + require.ErrorIs(t, err, context.Canceled) + assert.Equal(t, 1, attempts) +} diff --git a/cli/azd/pkg/ux/confirm.go b/cli/azd/pkg/ux/confirm.go index 6fdf419446a..1e11a516964 100644 --- a/cli/azd/pkg/ux/confirm.go +++ b/cli/azd/pkg/ux/confirm.go @@ -94,7 +94,7 @@ func NewConfirm(options *ConfirmOptions) *Confirm { } return &Confirm{ - input: internal.NewInput(), + input: internal.NewInput(mergedOptions.Writer), options: &mergedOptions, displayValue: displayValue, value: mergedOptions.DefaultValue, diff --git a/cli/azd/pkg/ux/input_writer_test.go b/cli/azd/pkg/ux/input_writer_test.go new file mode 100644 index 00000000000..16877655a0a --- /dev/null +++ b/cli/azd/pkg/ux/input_writer_test.go @@ -0,0 +1,87 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package ux + +import ( + "bytes" + "context" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// ansiShowCursor is the ANSI control sequence that makes the cursor visible; +// components emit it to restore the cursor after prompting. +const ansiShowCursor = "\033[?25h" + +func TestInputCursorUsesComponentWriter(t *testing.T) { + // Select and MultiSelect also restore their component cursor when Ask returns. + tests := []struct { + name string + expectedShowCursorCount int + ask func(context.Context, *bytes.Buffer) error + }{ + { + name: "Confirm", + expectedShowCursorCount: 1, + ask: func(ctx context.Context, writer *bytes.Buffer) error { + confirm := NewConfirm(&ConfirmOptions{Writer: writer}) + confirm.WithCanvas(NewCanvas().WithWriter(writer)) + _, err := confirm.Ask(ctx) + return err + }, + }, + { + name: "Prompt", + expectedShowCursorCount: 1, + ask: func(ctx context.Context, writer *bytes.Buffer) error { + prompt := NewPrompt(&PromptOptions{Writer: writer}) + prompt.WithCanvas(NewCanvas().WithWriter(writer)) + _, err := prompt.Ask(ctx) + return err + }, + }, + { + name: "Select", + expectedShowCursorCount: 2, + ask: func(ctx context.Context, writer *bytes.Buffer) error { + selectPrompt := NewSelect(&SelectOptions{ + Writer: writer, + Choices: []*SelectChoice{{Value: "one", Label: "One"}}, + }) + selectPrompt.WithCanvas(NewCanvas().WithWriter(writer)) + _, err := selectPrompt.Ask(ctx) + return err + }, + }, + { + name: "MultiSelect", + expectedShowCursorCount: 2, + ask: func(ctx context.Context, writer *bytes.Buffer) error { + multiSelect := NewMultiSelect(&MultiSelectOptions{ + Writer: writer, + Choices: []*MultiSelectChoice{{Value: "one", Label: "One"}}, + }) + multiSelect.WithCanvas(NewCanvas().WithWriter(writer)) + _, err := multiSelect.Ask(ctx) + return err + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + ctx, cancel := context.WithCancel(t.Context()) + cancel() + + var writer bytes.Buffer + err := test.ask(ctx, &writer) + + require.Error(t, err) + assert.Equal(t, test.expectedShowCursorCount, strings.Count(writer.String(), ansiShowCursor)) + }) + } +} diff --git a/cli/azd/pkg/ux/multi_select.go b/cli/azd/pkg/ux/multi_select.go index f8dc20043be..dd072aba461 100644 --- a/cli/azd/pkg/ux/multi_select.go +++ b/cli/azd/pkg/ux/multi_select.go @@ -119,7 +119,7 @@ func NewMultiSelect(options *MultiSelectOptions) *MultiSelect { } return &MultiSelect{ - input: internal.NewInput(), + input: internal.NewInput(mergedOptions.Writer), cursor: internal.NewCursor(mergedOptions.Writer), options: &mergedOptions, filteredChoices: selectOptions, diff --git a/cli/azd/pkg/ux/prompt.go b/cli/azd/pkg/ux/prompt.go index 3fae10a4955..23d2431f627 100644 --- a/cli/azd/pkg/ux/prompt.go +++ b/cli/azd/pkg/ux/prompt.go @@ -106,7 +106,7 @@ func NewPrompt(options *PromptOptions) *Prompt { } return &Prompt{ - input: internal.NewInput(), + input: internal.NewInput(mergedOptions.Writer), options: &mergedOptions, value: mergedOptions.DefaultValue, } diff --git a/cli/azd/pkg/ux/select.go b/cli/azd/pkg/ux/select.go index 7e5a8de904e..2c36209cf1b 100644 --- a/cli/azd/pkg/ux/select.go +++ b/cli/azd/pkg/ux/select.go @@ -112,7 +112,7 @@ func NewSelect(options *SelectOptions) *Select { } return &Select{ - input: internal.NewInput(), + input: internal.NewInput(mergedOptions.Writer), cursor: internal.NewCursor(mergedOptions.Writer), options: &mergedOptions, filteredChoices: selectOptions, diff --git a/cli/azd/test/functional/cli_test.go b/cli/azd/test/functional/cli_test.go index f5c99deef41..22cb38ee203 100644 --- a/cli/azd/test/functional/cli_test.go +++ b/cli/azd/test/functional/cli_test.go @@ -23,7 +23,6 @@ import ( "slices" "strings" "testing" - "time" "github.com/Azure/azure-sdk-for-go/sdk/azcore" "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" @@ -44,7 +43,6 @@ import ( "github.com/azure/azure-dev/cli/azd/test/recording" "github.com/benbjohnson/clock" "github.com/joho/godotenv" - "github.com/sethvargo/go-retry" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.opentelemetry.io/otel/attribute" @@ -1018,7 +1016,7 @@ func tempDirWithDiagnostics(t *testing.T) string { if runtime.GOOS == "windows" { // Enable our additional custom remove logic for Windows where we see locked files. t.Cleanup(func() { - err := removeAllWithDiagnostics(t, temp) + err := osutil.RemoveAll(context.Background(), temp) if err != nil { logHandles(t, temp) t.Fatalf("TempDirWithDiagnostics: %s", err) @@ -1061,29 +1059,6 @@ func logHandles(t *testing.T, path string) { span.End() } -func removeAllWithDiagnostics(t *testing.T, path string) error { - retryCount := 0 - loggedOnce := false - return retry.Do(context.Background(), retry.WithMaxRetries(10, retry.NewConstant(1*time.Second)), - func(_ context.Context) error { - removeErr := os.RemoveAll(path) - if removeErr == nil { - return nil - } - t.Logf("failed to clean up %s with error: %v", path, removeErr) - - if retryCount >= 2 && !loggedOnce { - // Only log once after 2 seconds - logHandles is pretty expensive and slow - logHandles(t, path) - loggedOnce = true - } - - retryCount++ - return retry.RetryableError(removeErr) - }, - ) -} - // Assert that all supported types from the infrastructure provider is marshalled and stored correctly in the environment. func assertEnvValuesStored(t *testing.T, env *environment.Environment) { expectedEnv, err := godotenv.Read(filepath.Join("testdata", "expected-output-types", "typed-values.env")) diff --git a/cli/azd/test/functional/extension_test.go b/cli/azd/test/functional/extension_test.go index 82f1b2e02d5..70ee6b42db0 100644 --- a/cli/azd/test/functional/extension_test.go +++ b/cli/azd/test/functional/extension_test.go @@ -11,6 +11,7 @@ import ( "strings" "testing" + "github.com/azure/azure-dev/cli/azd/pkg/osutil" "github.com/azure/azure-dev/cli/azd/test/azdcli" "github.com/azure/azure-dev/cli/azd/test/recording" "github.com/stretchr/testify/require" @@ -27,8 +28,12 @@ func Test_CLI_Extension_ForceInstall(t *testing.T) { ctx, cancel := newTestContext(t) defer cancel() + configDir := tempDirWithDiagnostics(t) cli := azdcli.NewCLI(t) - cli.Env = append(cli.Env, os.Environ()...) + cli.Env = append(os.Environ(), + "AZD_CONFIG_DIR="+configDir, + "AZURE_DEV_COLLECT_TELEMETRY=no", + ) // Setup: Add local extension source sourcePath := azdcli.GetSourcePath() @@ -40,9 +45,13 @@ func Test_CLI_Extension_ForceInstall(t *testing.T) { // Cleanup function to ensure extension is uninstalled and source removed defer func() { t.Log("Cleaning up: uninstalling microsoft.azd.demo extension") - _, _ = cli.RunCommand(ctx, "ext", "uninstall", "microsoft.azd.demo") + if _, err := cli.RunCommand(ctx, "ext", "uninstall", "microsoft.azd.demo"); err != nil { + t.Logf("warning: failed to uninstall microsoft.azd.demo: %v", err) + } t.Log("Cleaning up: removing test-local source") - _, _ = cli.RunCommand(ctx, "ext", "source", "remove", "test-local") + if _, err := cli.RunCommand(ctx, "ext", "source", "remove", "test-local"); err != nil { + t.Logf("warning: failed to remove test-local source: %v", err) + } }() // Step 1: Install the latest version of microsoft.azd.demo extension @@ -104,13 +113,11 @@ func Test_CLI_Extension_ForceInstall(t *testing.T) { t.Logf("Testing reinstall of same version (%s) with --force", targetVersion) // Get the extension binary path before deletion - homeDir, err := os.UserHomeDir() - require.NoError(t, err) - extPath := filepath.Join(homeDir, ".azd", "extensions", "microsoft.azd.demo") + extPath := filepath.Join(configDir, "extensions", "microsoft.azd.demo") // Delete the extension files but keep the metadata t.Log("Deleting extension files to simulate corruption") - err = os.RemoveAll(extPath) + err = osutil.RemoveAll(t.Context(), extPath) require.NoError(t, err) // Try to install without --force (should skip) diff --git a/cli/azd/test/functional/up_test.go b/cli/azd/test/functional/up_test.go index d5d5b705702..4fc808c2aeb 100644 --- a/cli/azd/test/functional/up_test.go +++ b/cli/azd/test/functional/up_test.go @@ -191,10 +191,14 @@ func Test_CLI_Up_Down_FuncApp(t *testing.T) { require.NoError(t, err) if session != nil { - session.Variables[recording.SubscriptionIdKey] = envValues[environment.SubscriptionIdEnvVarName].(string) + subID, ok := envValues[environment.SubscriptionIdEnvVarName].(string) + require.True(t, ok, "AZURE_SUBSCRIPTION_ID should be a string in env values") + session.Variables[recording.SubscriptionIdKey] = subID } - url := fmt.Sprintf("%s/api/httptrigger", envValues["AZURE_FUNCTION_URI"]) + funcURI, ok := envValues["AZURE_FUNCTION_URI"].(string) + require.True(t, ok, "AZURE_FUNCTION_URI should be a string in env values") + url := fmt.Sprintf("%s/api/httptrigger", funcURI) t.Logf("Issuing GET request to function\n") // We've seen some cases in CI where issuing a get right after a deploy ends up with us getting a 404, so retry the diff --git a/docs/guides/adding-a-new-command.md b/docs/guides/adding-a-new-command.md index 76c96c70a99..889b90e0032 100644 --- a/docs/guides/adding-a-new-command.md +++ b/docs/guides/adding-a-new-command.md @@ -88,6 +88,13 @@ After adding the command, regenerate CLI snapshots: UPDATE_SNAPSHOTS=true go test ./cmd -run 'TestFigSpec|TestUsage' ``` +### 5. Check reliability boundaries + +- Pass the injected `io.Writer` to nested formatters, spinners, and prompts. For terminal UX changes, verify + `go test -json` contains only JSON events. +- Functional tests that modify user-level state must use an isolated `AZD_CONFIG_DIR`. See + [Recording functional tests](../../cli/azd/docs/recording-functional-tests-guide.md). + ## Design Principles - **Verb-first structure:** Use simple verbs (`up`, `add`, `deploy`) with minimal nesting