From 6632960e883983940278d80de8d1fa7a7af27cca Mon Sep 17 00:00:00 2001 From: Nick Fedor Date: Sat, 28 Feb 2026 04:27:36 -0700 Subject: [PATCH 01/20] fix(tui): migrate to Bubble Tea v2 API - Update dependencies to charm.land/bubbletea/v2 and lipgloss/v2 - Change View() signature from string to tea.View return type - Replace tea.WithAltScreen() option with view.AltScreen field - Remove tea.EnterAltScreen command from Init() method - Update tests to use new tea.KeyPressMsg interface - Add Windows build exclusions for riscv64 and arm architectures --- .goreleaser.yaml | 6 +++ go.mod | 38 +++++++--------- go.sum | 68 +++++++++++++--------------- internal/cli/cli_test.go | 2 +- internal/cli/mocks/ProgramRunner.go | 2 +- internal/cli/tui.go | 24 ++++++---- internal/cli/tui_test.go | 69 ++++++++++++++++++++--------- 7 files changed, 120 insertions(+), 89 deletions(-) diff --git a/.goreleaser.yaml b/.goreleaser.yaml index dc97721..360c3df 100644 --- a/.goreleaser.yaml +++ b/.goreleaser.yaml @@ -16,6 +16,12 @@ builds: - arm - arm64 + ignore: + - goos: windows + goarch: riscv64 + - goos: windows + goarch: arm + archives: - name_template: >- {{- .ProjectName }}_ diff --git a/go.mod b/go.mod index 6c770fb..ef1bd22 100644 --- a/go.mod +++ b/go.mod @@ -3,40 +3,34 @@ module github.com/nicholas-fedor/go-remove go 1.26.0 require ( - github.com/charmbracelet/bubbletea v1.3.10 - github.com/charmbracelet/lipgloss v1.1.0 + charm.land/bubbletea/v2 v2.0.0 + charm.land/lipgloss/v2 v2.0.0 + github.com/spf13/cobra v1.10.2 github.com/stretchr/testify v1.11.1 go.uber.org/zap v1.27.1 ) require ( - github.com/clipperhouse/uax29/v2 v2.2.0 // indirect - github.com/inconshreveable/mousetrap v1.1.0 // indirect - github.com/spf13/pflag v1.0.10 // indirect -) - -require ( - github.com/aymanbagabas/go-osc52/v2 v2.0.1 // indirect - github.com/charmbracelet/colorprofile v0.3.2 // indirect - github.com/charmbracelet/x/ansi v0.10.2 // indirect - github.com/charmbracelet/x/cellbuf v0.0.13 // indirect - github.com/charmbracelet/x/term v0.2.1 // indirect + github.com/charmbracelet/colorprofile v0.4.2 // indirect + github.com/charmbracelet/ultraviolet v0.0.0-20260223171050-89c142e4aa73 // indirect + github.com/charmbracelet/x/ansi v0.11.6 // indirect + github.com/charmbracelet/x/term v0.2.2 // indirect + github.com/charmbracelet/x/termios v0.1.1 // indirect + github.com/charmbracelet/x/windows v0.2.2 // indirect + github.com/clipperhouse/displaywidth v0.11.0 // indirect + github.com/clipperhouse/uax29/v2 v2.7.0 // indirect github.com/davecgh/go-spew v1.1.1 // indirect - github.com/erikgeiser/coninput v0.0.0-20211004153227-1c3628e74d0f // indirect + github.com/inconshreveable/mousetrap v1.1.0 // indirect github.com/lucasb-eyer/go-colorful v1.3.0 // indirect - github.com/mattn/go-isatty v0.0.20 // indirect - github.com/mattn/go-localereader v0.0.1 // indirect - github.com/mattn/go-runewidth v0.0.19 // indirect - github.com/muesli/ansi v0.0.0-20230316100256-276c6243b2f6 // indirect + github.com/mattn/go-runewidth v0.0.20 // indirect github.com/muesli/cancelreader v0.2.2 // indirect - github.com/muesli/termenv v0.16.0 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect github.com/rivo/uniseg v0.4.7 // indirect - github.com/spf13/cobra v1.10.2 + github.com/spf13/pflag v1.0.10 // indirect github.com/stretchr/objx v0.5.3 // indirect github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e // indirect go.uber.org/multierr v1.11.0 // indirect - golang.org/x/sys v0.37.0 // indirect - golang.org/x/text v0.30.0 // indirect + golang.org/x/sync v0.19.0 // indirect + golang.org/x/sys v0.41.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect ) diff --git a/go.sum b/go.sum index 7ef2007..667e38c 100644 --- a/go.sum +++ b/go.sum @@ -1,40 +1,38 @@ -github.com/aymanbagabas/go-osc52/v2 v2.0.1 h1:HwpRHbFMcZLEVr42D4p7XBqjyuxQH5SMiErDT4WkJ2k= -github.com/aymanbagabas/go-osc52/v2 v2.0.1/go.mod h1:uYgXzlJ7ZpABp8OJ+exZzJJhRNQ2ASbcXHWsFqH8hp8= -github.com/charmbracelet/bubbletea v1.3.10 h1:otUDHWMMzQSB0Pkc87rm691KZ3SWa4KUlvF9nRvCICw= -github.com/charmbracelet/bubbletea v1.3.10/go.mod h1:ORQfo0fk8U+po9VaNvnV95UPWA1BitP1E0N6xJPlHr4= -github.com/charmbracelet/colorprofile v0.3.2 h1:9J27WdztfJQVAQKX2WOlSSRB+5gaKqqITmrvb1uTIiI= -github.com/charmbracelet/colorprofile v0.3.2/go.mod h1:mTD5XzNeWHj8oqHb+S1bssQb7vIHbepiebQ2kPKVKbI= -github.com/charmbracelet/lipgloss v1.1.0 h1:vYXsiLHVkK7fp74RkV7b2kq9+zDLoEU4MZoFqR/noCY= -github.com/charmbracelet/lipgloss v1.1.0/go.mod h1:/6Q8FR2o+kj8rz4Dq0zQc3vYf7X+B0binUUBwA0aL30= -github.com/charmbracelet/x/ansi v0.10.2 h1:ith2ArZS0CJG30cIUfID1LXN7ZFXRCww6RUvAPA+Pzw= -github.com/charmbracelet/x/ansi v0.10.2/go.mod h1:HbLdJjQH4UH4AqA2HpRWuWNluRE6zxJH/yteYEYCFa8= -github.com/charmbracelet/x/cellbuf v0.0.13 h1:/KBBKHuVRbq1lYx5BzEHBAFBP8VcQzJejZ/IA3iR28k= -github.com/charmbracelet/x/cellbuf v0.0.13/go.mod h1:xe0nKWGd3eJgtqZRaN9RjMtK7xUYchjzPr7q6kcvCCs= -github.com/charmbracelet/x/term v0.2.1 h1:AQeHeLZ1OqSXhrAWpYUtZyX1T3zVxfpZuEQMIQaGIAQ= -github.com/charmbracelet/x/term v0.2.1/go.mod h1:oQ4enTYFV7QN4m0i9mzHrViD7TQKvNEEkHUMCmsxdUg= -github.com/clipperhouse/uax29/v2 v2.2.0 h1:ChwIKnQN3kcZteTXMgb1wztSgaU+ZemkgWdohwgs8tY= -github.com/clipperhouse/uax29/v2 v2.2.0/go.mod h1:EFJ2TJMRUaplDxHKj1qAEhCtQPW2tJSwu5BF98AuoVM= +charm.land/bubbletea/v2 v2.0.0 h1:p0d6CtWyJXJ9GfzMpUUqbP/XUUhhlk06+vCKWmox1wQ= +charm.land/bubbletea/v2 v2.0.0/go.mod h1:3LRff2U4WIYXy7MTxfbAQ+AdfM3D8Xuvz2wbsOD9OHQ= +charm.land/lipgloss/v2 v2.0.0 h1:sd8N/B3x892oiOjFfBQdXBQp3cAkvjGaU5TvVZC3ivo= +charm.land/lipgloss/v2 v2.0.0/go.mod h1:w6SnmsBFBmEFBodiEDurGS/sdUY/u1+v72DqUzc6J14= +github.com/aymanbagabas/go-udiff v0.4.0 h1:TKnLPh7IbnizJIBKFWa9mKayRUBQ9Kh1BPCk6w2PnYM= +github.com/aymanbagabas/go-udiff v0.4.0/go.mod h1:0L9PGwj20lrtmEMeyw4WKJ/TMyDtvAoK9bf2u/mNo3w= +github.com/charmbracelet/colorprofile v0.4.2 h1:BdSNuMjRbotnxHSfxy+PCSa4xAmz7szw70ktAtWRYrY= +github.com/charmbracelet/colorprofile v0.4.2/go.mod h1:0rTi81QpwDElInthtrQ6Ni7cG0sDtwAd4C4le060fT8= +github.com/charmbracelet/ultraviolet v0.0.0-20260223171050-89c142e4aa73 h1:Af/L28Xh+pddhouT/6lJ7IAIYfu5tWJOB0iqt+mXsYM= +github.com/charmbracelet/ultraviolet v0.0.0-20260223171050-89c142e4aa73/go.mod h1:E6/0abq9uG2SnM8IbLB9Y5SW09uIgfaFETk8aRzgXUQ= +github.com/charmbracelet/x/ansi v0.11.6 h1:GhV21SiDz/45W9AnV2R61xZMRri5NlLnl6CVF7ihZW8= +github.com/charmbracelet/x/ansi v0.11.6/go.mod h1:2JNYLgQUsyqaiLovhU2Rv/pb8r6ydXKS3NIttu3VGZQ= +github.com/charmbracelet/x/exp/golden v0.0.0-20250806222409-83e3a29d542f h1:pk6gmGpCE7F3FcjaOEKYriCvpmIN4+6OS/RD0vm4uIA= +github.com/charmbracelet/x/exp/golden v0.0.0-20250806222409-83e3a29d542f/go.mod h1:IfZAMTHB6XkZSeXUqriemErjAWCCzT0LwjKFYCZyw0I= +github.com/charmbracelet/x/term v0.2.2 h1:xVRT/S2ZcKdhhOuSP4t5cLi5o+JxklsoEObBSgfgZRk= +github.com/charmbracelet/x/term v0.2.2/go.mod h1:kF8CY5RddLWrsgVwpw4kAa6TESp6EB5y3uxGLeCqzAI= +github.com/charmbracelet/x/termios v0.1.1 h1:o3Q2bT8eqzGnGPOYheoYS8eEleT5ZVNYNy8JawjaNZY= +github.com/charmbracelet/x/termios v0.1.1/go.mod h1:rB7fnv1TgOPOyyKRJ9o+AsTU/vK5WHJ2ivHeut/Pcwo= +github.com/charmbracelet/x/windows v0.2.2 h1:IofanmuvaxnKHuV04sC0eBy/smG6kIKrWG2/jYn2GuM= +github.com/charmbracelet/x/windows v0.2.2/go.mod h1:/8XtdKZzedat74NQFn0NGlGL4soHB0YQZrETF96h75k= +github.com/clipperhouse/displaywidth v0.11.0 h1:lBc6kY44VFw+TDx4I8opi/EtL9m20WSEFgwIwO+UVM8= +github.com/clipperhouse/displaywidth v0.11.0/go.mod h1:bkrFNkf81G8HyVqmKGxsPufD3JhNl3dSqnGhOoSD/o0= +github.com/clipperhouse/uax29/v2 v2.7.0 h1:+gs4oBZ2gPfVrKPthwbMzWZDaAFPGYK72F0NJv2v7Vk= +github.com/clipperhouse/uax29/v2 v2.7.0/go.mod h1:EFJ2TJMRUaplDxHKj1qAEhCtQPW2tJSwu5BF98AuoVM= github.com/cpuguy83/go-md2man/v2 v2.0.6/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= -github.com/erikgeiser/coninput v0.0.0-20211004153227-1c3628e74d0f h1:Y/CXytFA4m6baUTXGLOoWe4PQhGxaX0KpnayAqC48p4= -github.com/erikgeiser/coninput v0.0.0-20211004153227-1c3628e74d0f/go.mod h1:vw97MGsxSvLiUE2X8qFplwetxpGLQrlU1Q9AUEIzCaM= github.com/inconshreveable/mousetrap v1.1.0 h1:wN+x4NVGpMsO7ErUn/mUI3vEoE6Jt13X2s0bqwp9tc8= github.com/inconshreveable/mousetrap v1.1.0/go.mod h1:vpF70FUmC8bwa3OWnCshd2FqLfsEA9PFc4w1p2J65bw= github.com/lucasb-eyer/go-colorful v1.3.0 h1:2/yBRLdWBZKrf7gB40FoiKfAWYQ0lqNcbuQwVHXptag= github.com/lucasb-eyer/go-colorful v1.3.0/go.mod h1:R4dSotOR9KMtayYi1e77YzuveK+i7ruzyGqttikkLy0= -github.com/mattn/go-isatty v0.0.20 h1:xfD0iDuEKnDkl03q4limB+vH+GxLEtL/jb4xVJSWWEY= -github.com/mattn/go-isatty v0.0.20/go.mod h1:W+V8PltTTMOvKvAeJH7IuucS94S2C6jfK/D7dTCTo3Y= -github.com/mattn/go-localereader v0.0.1 h1:ygSAOl7ZXTx4RdPYinUpg6W99U8jWvWi9Ye2JC/oIi4= -github.com/mattn/go-localereader v0.0.1/go.mod h1:8fBrzywKY7BI3czFoHkuzRoWE9C+EiG4R1k4Cjx5p88= -github.com/mattn/go-runewidth v0.0.19 h1:v++JhqYnZuu5jSKrk9RbgF5v4CGUjqRfBm05byFGLdw= -github.com/mattn/go-runewidth v0.0.19/go.mod h1:XBkDxAl56ILZc9knddidhrOlY5R/pDhgLpndooCuJAs= -github.com/muesli/ansi v0.0.0-20230316100256-276c6243b2f6 h1:ZK8zHtRHOkbHy6Mmr5D264iyp3TiX5OmNcI5cIARiQI= -github.com/muesli/ansi v0.0.0-20230316100256-276c6243b2f6/go.mod h1:CJlz5H+gyd6CUWT45Oy4q24RdLyn7Md9Vj2/ldJBSIo= +github.com/mattn/go-runewidth v0.0.20 h1:WcT52H91ZUAwy8+HUkdM3THM6gXqXuLJi9O3rjcQQaQ= +github.com/mattn/go-runewidth v0.0.20/go.mod h1:XBkDxAl56ILZc9knddidhrOlY5R/pDhgLpndooCuJAs= github.com/muesli/cancelreader v0.2.2 h1:3I4Kt4BQjOR54NavqnDogx/MIoWBFa0StPA8ELUXHmA= github.com/muesli/cancelreader v0.2.2/go.mod h1:3XuTXfFS2VjM+HTLZY9Ak0l6eUKfijIfMUZ4EgX0QYo= -github.com/muesli/termenv v0.16.0 h1:S5AlUN9dENB57rsbnkPyfdGuWIlkmzJjbFf0Tf5FWUc= -github.com/muesli/termenv v0.16.0/go.mod h1:ZRfOIKPFDYQoDFF4Olj7/QJbW60Ol/kL1pU3VfY/Cnk= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/rivo/uniseg v0.4.7 h1:WUdvkW8uEhrYfLC4ZzdpI2ztxP1I582+49Oc5Mq64VQ= @@ -58,14 +56,12 @@ go.uber.org/multierr v1.11.0/go.mod h1:20+QtiLqy0Nd6FdQB9TLXag12DsQkrbs3htMFfDN8 go.uber.org/zap v1.27.1 h1:08RqriUEv8+ArZRYSTXy1LeBScaMpVSTBhCeaZYfMYc= go.uber.org/zap v1.27.1/go.mod h1:GB2qFLM7cTU87MWRP2mPIjqfIDnGu+VIO4V/SdhGo2E= go.yaml.in/yaml/v3 v3.0.4/go.mod h1:DhzuOOF2ATzADvBadXxruRBLzYTpT36CKvDb3+aBEFg= -golang.org/x/exp v0.0.0-20220909182711-5c715a9e8561 h1:MDc5xs78ZrZr3HMQugiXOAkSZtfTpbJLDr/lwfgO53E= -golang.org/x/exp v0.0.0-20220909182711-5c715a9e8561/go.mod h1:cyybsKvd6eL0RnXn6p/Grxp8F5bW7iYuBgsNCOHpMYE= -golang.org/x/sys v0.0.0-20210809222454-d867a43fc93e/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.37.0 h1:fdNQudmxPjkdUTPnLn5mdQv7Zwvbvpaxqs831goi9kQ= -golang.org/x/sys v0.37.0/go.mod h1:OgkHotnGiDImocRcuBABYBEXf8A9a87e/uXjp9XT3ks= -golang.org/x/text v0.30.0 h1:yznKA/E9zq54KzlzBEAWn1NXSQ8DIp/NYMy88xJjl4k= -golang.org/x/text v0.30.0/go.mod h1:yDdHFIX9t+tORqspjENWgzaCVXgk0yYnYuSZ8UzzBVM= +golang.org/x/exp v0.0.0-20231006140011-7918f672742d h1:jtJma62tbqLibJ5sFQz8bKtEM8rJBtfilJ2qTU199MI= +golang.org/x/exp v0.0.0-20231006140011-7918f672742d/go.mod h1:ldy0pHrwJyGW56pPQzzkH36rKxoZW1tw7ZJpeKx+hdo= +golang.org/x/sync v0.19.0 h1:vV+1eWNmZ5geRlYjzm2adRgW2/mcpevXNg50YZtPCE4= +golang.org/x/sync v0.19.0/go.mod h1:9KTHXmSnoGruLpwFjVSX0lNNA75CykiMECbovNTZqGI= +golang.org/x/sys v0.41.0 h1:Ivj+2Cp/ylzLiEU89QhWblYnOE9zerudt9Ftecq2C6k= +golang.org/x/sys v0.41.0/go.mod h1:OgkHotnGiDImocRcuBABYBEXf8A9a87e/uXjp9XT3ks= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index 1c0e7c4..8a6b60e 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -28,7 +28,7 @@ import ( "go.uber.org/zap" "go.uber.org/zap/zapcore" - tea "github.com/charmbracelet/bubbletea" + tea "charm.land/bubbletea/v2" fsmocks "github.com/nicholas-fedor/go-remove/internal/fs/mocks" "github.com/nicholas-fedor/go-remove/internal/logger" diff --git a/internal/cli/mocks/ProgramRunner.go b/internal/cli/mocks/ProgramRunner.go index 609ef70..dec7538 100644 --- a/internal/cli/mocks/ProgramRunner.go +++ b/internal/cli/mocks/ProgramRunner.go @@ -5,7 +5,7 @@ package mocks import ( - "github.com/charmbracelet/bubbletea" + "charm.land/bubbletea/v2" mock "github.com/stretchr/testify/mock" ) diff --git a/internal/cli/tui.go b/internal/cli/tui.go index 9d3c2ef..dafeb17 100644 --- a/internal/cli/tui.go +++ b/internal/cli/tui.go @@ -24,9 +24,9 @@ import ( "sort" "strings" - "github.com/charmbracelet/lipgloss" + "charm.land/lipgloss/v2" - tea "github.com/charmbracelet/bubbletea" + tea "charm.land/bubbletea/v2" "github.com/nicholas-fedor/go-remove/internal/fs" "github.com/nicholas-fedor/go-remove/internal/logger" @@ -99,7 +99,7 @@ func RunTUI(dir string, config Config, logger logger.Logger, fs fs.FS, runner Pr cursorY: 0, sortAscending: true, styles: defaultStyleConfig(), - }, tea.WithAltScreen()) + }) if err != nil { return fmt.Errorf("failed to start TUI program: %w", err) } @@ -140,7 +140,7 @@ func (r DefaultRunner) RunProgram(m tea.Model, opts ...tea.ProgramOption) (*tea. func (m *model) Init() tea.Cmd { m.sortChoices() - return tea.EnterAltScreen // Switch to alternate screen buffer + return nil } // Update processes TUI events and updates the model state. @@ -300,10 +300,13 @@ func (m *model) updateGrid() { } } -// View renders the TUI interface as a string. -func (m *model) View() string { +// View renders the TUI interface as a tea.View. +func (m *model) View() tea.View { if len(m.choices) == 0 { - return "No binaries found.\n" + view := tea.NewView("No binaries found.\n") + view.AltScreen = true + + return view } // Apply configured styles for UI elements. @@ -378,10 +381,15 @@ func (m *model) View() string { s.WriteString(footer) - return lipgloss.NewStyle(). + content := lipgloss.NewStyle(). PaddingLeft(leftPadding). Width(m.width - leftPadding). Render(s.String()) + + view := tea.NewView(content) + view.AltScreen = true + + return view } // maximum returns the larger of two integers. diff --git a/internal/cli/tui_test.go b/internal/cli/tui_test.go index 6c61bbc..d4965c4 100644 --- a/internal/cli/tui_test.go +++ b/internal/cli/tui_test.go @@ -27,7 +27,7 @@ import ( "github.com/stretchr/testify/mock" - tea "github.com/charmbracelet/bubbletea" + tea "charm.land/bubbletea/v2" fsmocks "github.com/nicholas-fedor/go-remove/internal/fs/mocks" logmocks "github.com/nicholas-fedor/go-remove/internal/logger/mocks" @@ -38,7 +38,7 @@ type tuiMockRunner struct { runProgram func(tea.Model, ...tea.ProgramOption) (*tea.Program, error) } -// RunProgram executes the mock runner’s program function. +// RunProgram executes the mock runner's program function. func (m *tuiMockRunner) RunProgram( model tea.Model, opts ...tea.ProgramOption, @@ -46,7 +46,7 @@ func (m *tuiMockRunner) RunProgram( return m.runProgram(model, opts...) } -// TestRunTUI verifies the RunTUI function’s behavior under various conditions. +// TestRunTUI verifies the RunTUI function's behavior under various conditions. func TestRunTUI(t *testing.T) { type args struct { dir string @@ -128,7 +128,7 @@ func TestRunTUI(t *testing.T) { } } -// Test_model_Init verifies the Init method’s command output. +// Test_model_Init verifies the Init method's command output. func Test_model_Init(t *testing.T) { tests := []struct { name string @@ -138,7 +138,7 @@ func Test_model_Init(t *testing.T) { { name: "basic init", m: model{}, - want: tea.EnterAltScreen, + want: nil, }, } @@ -152,7 +152,34 @@ func Test_model_Init(t *testing.T) { } } -// Test_model_Update verifies the Update method’s state changes and commands. +// keyPress creates a KeyPressMsg with the given rune. +func keyPress(r rune) tea.KeyPressMsg { + return tea.KeyPressMsg{Text: string(r), Code: r, ShiftedCode: r} +} + +// keyPressString creates a KeyPressMsg from a string representation. +func keyPressString(s string) tea.KeyPressMsg { + switch s { + case "enter": + return tea.KeyPressMsg{Code: tea.KeyEnter} + case "up": + return tea.KeyPressMsg{Code: tea.KeyUp} + case "down": + return tea.KeyPressMsg{Code: tea.KeyDown} + case "left": + return tea.KeyPressMsg{Code: tea.KeyLeft} + case "right": + return tea.KeyPressMsg{Code: tea.KeyRight} + default: + if len(s) == 1 { + r := rune(s[0]) + return tea.KeyPressMsg{Text: s, Code: r, ShiftedCode: r} + } + return tea.KeyPressMsg{} + } +} + +// Test_model_Update verifies the Update method's state changes and commands. func Test_model_Update(t *testing.T) { type args struct { msg tea.Msg @@ -168,21 +195,21 @@ func Test_model_Update(t *testing.T) { { name: "quit with q", m: &model{}, - args: args{msg: tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'q'}}}, + args: args{msg: keyPress('q')}, want: model{}, wantCmd: tea.Quit, }, { name: "move up", m: &model{cursorY: 1, rows: 2}, - args: args{msg: tea.KeyMsg{Type: tea.KeyUp}}, + args: args{msg: keyPressString("up")}, want: model{cursorY: 0, rows: 2}, wantCmd: nil, }, { name: "move down within bounds", m: &model{cursorY: 0, rows: 2, cols: 2, choices: []string{"a", "b", "c", "d"}}, - args: args{msg: tea.KeyMsg{Type: tea.KeyDown}}, + args: args{msg: keyPressString("down")}, want: model{cursorY: 1, rows: 2, cols: 2, choices: []string{"a", "b", "c", "d"}}, wantCmd: nil, }, @@ -195,7 +222,7 @@ func Test_model_Update(t *testing.T) { cols: 2, choices: []string{"a", "b", "c", "d"}, }, - args: args{msg: tea.KeyMsg{Type: tea.KeyDown}}, + args: args{msg: keyPressString("down")}, want: model{ cursorY: 1, cursorX: 1, @@ -208,14 +235,14 @@ func Test_model_Update(t *testing.T) { { name: "move left", m: &model{cursorX: 1, cols: 2}, - args: args{msg: tea.KeyMsg{Type: tea.KeyLeft}}, + args: args{msg: keyPressString("left")}, want: model{cursorX: 0, cols: 2}, wantCmd: nil, }, { name: "move right within bounds", m: &model{cursorX: 0, cols: 2, choices: []string{"a", "b"}}, - args: args{msg: tea.KeyMsg{Type: tea.KeyRight}}, + args: args{msg: keyPressString("right")}, want: model{cursorX: 1, cols: 2, choices: []string{"a", "b"}}, wantCmd: nil, }, @@ -229,7 +256,7 @@ func Test_model_Update(t *testing.T) { width: 80, height: 24, }, - args: args{msg: tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'s'}}}, + args: args{msg: keyPress('s')}, want: model{ choices: []string{"vhs", "age"}, sortAscending: false, @@ -250,7 +277,7 @@ func Test_model_Update(t *testing.T) { width: 80, height: 24, }, - args: args{msg: tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'s'}}}, + args: args{msg: keyPress('s')}, want: model{ choices: []string{"age", "vhs"}, sortAscending: true, @@ -286,7 +313,7 @@ func Test_model_Update(t *testing.T) { return m }(), }, - args: args{msg: tea.KeyMsg{Type: tea.KeyEnter}}, + args: args{msg: keyPressString("enter")}, want: model{ choices: []string{"vhs"}, cols: 1, @@ -324,7 +351,7 @@ func Test_model_Update(t *testing.T) { return m }(), }, - args: args{msg: tea.KeyMsg{Type: tea.KeyEnter}}, + args: args{msg: keyPressString("enter")}, want: model{ choices: []string{"age"}, cols: 1, @@ -390,7 +417,7 @@ func Test_model_Update(t *testing.T) { } } -// Test_model_updateGrid verifies the updateGrid method’s layout calculations. +// Test_model_updateGrid verifies the updateGrid method's layout calculations. func Test_model_updateGrid(t *testing.T) { tests := []struct { name string @@ -467,7 +494,7 @@ func stripANSI(str string) string { return ansiRegex.ReplaceAllString(str, "") } -// Test_model_View verifies the View method’s rendered output. +// Test_model_View verifies the View method's rendered output. func Test_model_View(t *testing.T) { const contentWidth = 78 // Max visible width for content (excluding leftPadding) @@ -583,7 +610,7 @@ func Test_model_View(t *testing.T) { tt.m.sortChoices() tt.m.updateGrid() - got := stripANSI(tt.m.View()) + got := stripANSI(tt.m.View().Content) if got != tt.want { t.Errorf("model.View() got = %q, want %q", got, tt.want) } @@ -591,7 +618,7 @@ func Test_model_View(t *testing.T) { } } -// Test_max verifies the max function’s comparison logic. +// Test_max verifies the max function's comparison logic. func Test_max(t *testing.T) { tests := []struct { name string @@ -613,7 +640,7 @@ func Test_max(t *testing.T) { } } -// Test_min verifies the min function’s comparison logic. +// Test_min verifies the min function's comparison logic. func Test_min(t *testing.T) { tests := []struct { name string From b74f8a1f9dc646b9b7de993ac36af5bb3c00ce3d Mon Sep 17 00:00:00 2001 From: Nick Fedor Date: Sat, 28 Feb 2026 04:37:21 -0700 Subject: [PATCH 02/20] ci(workflows): use go.mod for Go version management - Configure setup-go action to read Go version from go.mod file - Remove hardcoded GO_VERSION environment variables - Remove Go version matrix from test workflow --- .github/workflows/build.yaml | 2 +- .github/workflows/lint.yaml | 5 +---- .github/workflows/security.yaml | 3 +-- .github/workflows/test.yaml | 4 +--- 4 files changed, 4 insertions(+), 10 deletions(-) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index e685121..33b0bd9 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -25,7 +25,7 @@ jobs: uses: actions/setup-go@def8c394e3ad351a79bc93815e4a585520fe993b with: check-latest: true - go-version: 1.26.x + go-version-file: "go.mod" - name: Import GPG key uses: crazy-max/ghaction-import-gpg@5a30dd90ba97bcb89e03060b1d36736828e222e4 diff --git a/.github/workflows/lint.yaml b/.github/workflows/lint.yaml index 8be6937..d4bc1a7 100644 --- a/.github/workflows/lint.yaml +++ b/.github/workflows/lint.yaml @@ -6,9 +6,6 @@ on: permissions: contents: read -env: - GO_VERSION: 1.25.x - jobs: lint: name: Run Linter @@ -23,7 +20,7 @@ jobs: uses: actions/setup-go@def8c394e3ad351a79bc93815e4a585520fe993b with: check-latest: true - go-version: ${{ env.GO_VERSION }} + go-version-file: "go.mod" - name: Install dependencies run: go mod download diff --git a/.github/workflows/security.yaml b/.github/workflows/security.yaml index 322e6ad..c8127c7 100644 --- a/.github/workflows/security.yaml +++ b/.github/workflows/security.yaml @@ -19,7 +19,6 @@ permissions: security-events: write env: - GO_VERSION: 1.25.x OUTPUT_FILE: results.sarif jobs: @@ -48,7 +47,7 @@ jobs: with: output-format: sarif output-file: ${{ env.OUTPUT_FILE }} - go-version-input: ${{ env.GO_VERSION }} + go-version-file: "go.mod" - name: Upload SARIF file uses: github/codeql-action/upload-sarif@89a39a4e59826350b863aa6b6252a07ad50cf83e # v4 diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index d2ed050..cc70aef 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -13,8 +13,6 @@ jobs: strategy: fail-fast: false matrix: - go-version: - - 1.25.x platform: - macos-latest - windows-latest @@ -29,7 +27,7 @@ jobs: uses: actions/setup-go@def8c394e3ad351a79bc93815e4a585520fe993b with: check-latest: true - go-version: ${{ matrix.go-version }} + go-version-file: "go.mod" cache: true cache-dependency-path: "**/go.sum" From 64e37f056af9faf98311279004a921408a7798ff Mon Sep 17 00:00:00 2001 From: Nick Fedor Date: Sat, 28 Feb 2026 04:59:09 -0700 Subject: [PATCH 03/20] chore: clean up configuration files and documentation - Simplify project description in README - Update mockery configuration to use latest version - Add ./bin directory to gitignore - Remove redundant windows/riscv64 ignore from goreleaser --- .gitignore | 3 ++- .goreleaser.yaml | 2 -- .mockery.yaml | 3 +-- README.md | 3 +-- 4 files changed, 4 insertions(+), 7 deletions(-) diff --git a/.gitignore b/.gitignore index 9766e11..ca2434b 100644 --- a/.gitignore +++ b/.gitignore @@ -2,6 +2,7 @@ # https://github.com/github/gitignore/blob/main/community/Golang/Go.AllowList.gitignore # # Binaries for programs and plugins +./bin *.exe *.exe~ *.dll @@ -21,4 +22,4 @@ go.work # Ignore VSCode settings.json -.vscode/settings.json \ No newline at end of file +.vscode/settings.json diff --git a/.goreleaser.yaml b/.goreleaser.yaml index 360c3df..c691356 100644 --- a/.goreleaser.yaml +++ b/.goreleaser.yaml @@ -17,8 +17,6 @@ builds: - arm64 ignore: - - goos: windows - goarch: riscv64 - goos: windows goarch: arm diff --git a/.mockery.yaml b/.mockery.yaml index b10d426..56cf410 100644 --- a/.mockery.yaml +++ b/.mockery.yaml @@ -5,7 +5,6 @@ # go-remove: https://github.com/nicholas-fedor/go-remove/ # # Mockery: https://vektra.github.io/mockery/latest/ # # Configuration: https://vektra.github.io/mockery/latest/configuration # -# Mockery Version: 3.5.2 # # # ###################################################################################################### @@ -13,7 +12,7 @@ # Mockery Usage # # # # Install Mockery: # -# go install github.com/vektra/mockery/v3@v3.5.2 # +# go install github.com/vektra/mockery/v3@latest # # # # Run CLI command from root: # # mockery # diff --git a/README.md b/README.md index 9fd5945..6c8ade9 100644 --- a/README.md +++ b/README.md @@ -19,8 +19,7 @@ A command-line tool to remove Go binaries with ease -`go-remove` is a command-line tool for removing Go binaries from standard binary directories (`GOROOT/bin`, `GOBIN`, or `GOPATH/bin`). -It supports both direct removal of a specified binary and an interactive TUI (Terminal User Interface) mode for selecting binaries to delete. +`go-remove` is a simple CLI/TUI for removing Go binaries. ## Features From 4e0d77930b76445671e9e5b10f9da820a6487d9f Mon Sep 17 00:00:00 2001 From: Nick Fedor Date: Sat, 28 Feb 2026 05:02:11 -0700 Subject: [PATCH 04/20] chore: update linting configuration and fix compliance issues - Add gocritic and revive configurations to .golangci.yaml - Rename parameters to avoid package name conflicts - Refactor test assertions to use literal path strings - Apply consistent formatting with blank lines and append patterns --- .golangci.yaml | 20 ++++++++++++++++++++ cmd/root.go | 2 ++ internal/cli/tui.go | 14 ++++++++++---- internal/cli/tui_test.go | 39 ++++++++++++++++++++++----------------- internal/fs/fs.go | 4 ++-- internal/fs/fs_test.go | 8 ++++---- 6 files changed, 60 insertions(+), 27 deletions(-) diff --git a/.golangci.yaml b/.golangci.yaml index c778be5..abcbe34 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -131,6 +131,26 @@ linters: # Linter Settings Configuration ###################################################################################################### settings: + gocritic: + enable-all: true + disabled-checks: + - unnamedResult + revive: + rules: + - name: var-naming + severity: warning + disabled: false + exclude: [""] + arguments: + - [""] # AllowList + - [""] # DenyList + - - skip-initialism-name-checks: false + upper-case-const: true + skip-package-name-checks: true + skip-package-name-collision-with-go-std: true + extra-bad-package-names: + - helpers + - models varnamelen: max-distance: 5 min-name-length: 3 diff --git a/cmd/root.go b/cmd/root.go index d9f6c05..9c41f67 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -74,6 +74,7 @@ var rootCmd = &cobra.Command{ log, ) } + switch logLevel { case "debug": zapLogger.Logger = zapLogger.WithOptions( @@ -92,6 +93,7 @@ var rootCmd = &cobra.Command{ zap.IncreaseLevel(zapcore.InfoLevel), ) } + log = zapLogger // Update the logger in deps deps.Logger = log } diff --git a/internal/cli/tui.go b/internal/cli/tui.go index dafeb17..02bb09b 100644 --- a/internal/cli/tui.go +++ b/internal/cli/tui.go @@ -81,9 +81,15 @@ type model struct { type DefaultRunner struct{} // RunTUI launches the interactive TUI mode for binary selection and removal. -func RunTUI(dir string, config Config, logger logger.Logger, fs fs.FS, runner ProgramRunner) error { +func RunTUI( + dir string, + config Config, + log logger.Logger, + filesystem fs.FS, + runner ProgramRunner, +) error { // Fetch available binaries from the specified directory. - choices := fs.ListBinaries(dir) + choices := filesystem.ListBinaries(dir) if len(choices) == 0 { return fmt.Errorf("%w: %s", ErrNoBinariesFound, dir) } @@ -93,8 +99,8 @@ func RunTUI(dir string, config Config, logger logger.Logger, fs fs.FS, runner Pr choices: choices, dir: dir, config: config, - logger: logger, - fs: fs, + logger: log, + fs: filesystem, cursorX: 0, cursorY: 0, sortAscending: true, diff --git a/internal/cli/tui_test.go b/internal/cli/tui_test.go index d4965c4..d8edf4e 100644 --- a/internal/cli/tui_test.go +++ b/internal/cli/tui_test.go @@ -173,8 +173,10 @@ func keyPressString(s string) tea.KeyPressMsg { default: if len(s) == 1 { r := rune(s[0]) + return tea.KeyPressMsg{Text: s, Code: r, ShiftedCode: r} } + return tea.KeyPressMsg{} } } @@ -540,14 +542,14 @@ func Test_model_View(t *testing.T) { }, want: func() string { lines := make([]string, 0, 25) + lines = append( lines, leftPaddingStr+pad("Select a binary to remove:", effectiveWidth), + leftPaddingStr+pad("", effectiveWidth), + leftPaddingStr+pad("❯ vhs", effectiveWidth), + leftPaddingStr+pad("", effectiveWidth), ) - lines = append(lines, leftPaddingStr+pad("", effectiveWidth)) - lines = append(lines, leftPaddingStr+pad("❯ vhs", effectiveWidth)) - - lines = append(lines, leftPaddingStr+pad("", effectiveWidth)) for range 19 { lines = append(lines, leftPaddingStr+pad("", effectiveWidth)) } @@ -555,8 +557,11 @@ func Test_model_View(t *testing.T) { footerPart1 := "↑/k: up ↓/j: down ←/h: left →/l: right Enter: remove s: toggle sort q:" footerPart2 := "quit" - lines = append(lines, leftPaddingStr+pad(footerPart1, effectiveWidth)) - lines = append(lines, leftPaddingStr+pad(footerPart2, effectiveWidth)) + lines = append( + lines, + leftPaddingStr+pad(footerPart1, effectiveWidth), + leftPaddingStr+pad(footerPart2, effectiveWidth), + ) return strings.Join(lines, "\n") }(), @@ -577,19 +582,16 @@ func Test_model_View(t *testing.T) { }, want: func() string { lines := make([]string, 0, 25) + lines = append( lines, leftPaddingStr+pad("Select a binary to remove:", effectiveWidth), + leftPaddingStr+pad("", effectiveWidth), + leftPaddingStr+pad("❯ age", effectiveWidth), + leftPaddingStr+pad(" vhs", effectiveWidth), // Adjusted to match actual padding + leftPaddingStr+pad("", effectiveWidth), + leftPaddingStr+pad("Removed tool", effectiveWidth), ) - lines = append(lines, leftPaddingStr+pad("", effectiveWidth)) - lines = append(lines, leftPaddingStr+pad("❯ age", effectiveWidth)) - lines = append(lines, leftPaddingStr+pad( - " vhs", - effectiveWidth, - )) // Adjusted to match actual padding - lines = append(lines, leftPaddingStr+pad("", effectiveWidth)) - - lines = append(lines, leftPaddingStr+pad("Removed tool", effectiveWidth)) for range 17 { // Adjusted for rows=2 lines = append(lines, leftPaddingStr+pad("", effectiveWidth)) } @@ -597,8 +599,11 @@ func Test_model_View(t *testing.T) { footerPart1 := "↑/k: up ↓/j: down ←/h: left →/l: right Enter: remove s: toggle sort q:" footerPart2 := "quit" - lines = append(lines, leftPaddingStr+pad(footerPart1, effectiveWidth)) - lines = append(lines, leftPaddingStr+pad(footerPart2, effectiveWidth)) + lines = append( + lines, + leftPaddingStr+pad(footerPart1, effectiveWidth), + leftPaddingStr+pad(footerPart2, effectiveWidth), + ) return strings.Join(lines, "\n") }(), diff --git a/internal/fs/fs.go b/internal/fs/fs.go index 96701c2..82274b3 100644 --- a/internal/fs/fs.go +++ b/internal/fs/fs.go @@ -102,14 +102,14 @@ func (r *RealFS) AdjustBinaryPath(dir, binary string) string { } // RemoveBinary deletes a binary file from the filesystem. -func (r *RealFS) RemoveBinary(binaryPath, name string, verbose bool, logger logger.Logger) error { +func (r *RealFS) RemoveBinary(binaryPath, name string, verbose bool, log logger.Logger) error { // Verify the binary exists before attempting removal. if _, err := os.Stat(binaryPath); os.IsNotExist(err) { return fmt.Errorf("%w: %s at %s", ErrBinaryNotFound, name, binaryPath) } // Log debug and info messages if verbose mode is enabled. - sugar := logger.Sugar() + sugar := log.Sugar() if verbose { sugar.Debugf("Constructed binary path: %s", binaryPath) sugar.Infof("Removing binary: %s", binaryPath) diff --git a/internal/fs/fs_test.go b/internal/fs/fs_test.go index b1ce7d6..b892c9c 100644 --- a/internal/fs/fs_test.go +++ b/internal/fs/fs_test.go @@ -71,7 +71,7 @@ func TestRealFS_DetermineBinDir(t *testing.T) { r: &RealFS{}, args: args{useGoroot: true}, env: map[string]string{"GOROOT": "/go"}, - want: filepath.Join("/go", "bin"), + want: "/go/bin", wantErr: false, }, { @@ -95,7 +95,7 @@ func TestRealFS_DetermineBinDir(t *testing.T) { r: &RealFS{}, args: args{useGoroot: false}, env: map[string]string{"GOPATH": "/gopath", "GOBIN": ""}, - want: filepath.Join("/gopath", "bin"), + want: "/gopath/bin", wantErr: false, }, { @@ -161,7 +161,7 @@ func TestRealFS_AdjustBinaryPath(t *testing.T) { name: "basic path", r: &RealFS{}, args: args{dir: "/bin", binary: "tool"}, - want: filepath.Join("/bin", "tool") + func() string { + want: "/bin/tool" + func() string { if runtime.GOOS == windowsOS { return windowsExt } @@ -186,7 +186,7 @@ func TestRealFS_AdjustBinaryPath(t *testing.T) { name: "windows adds .exe", r: &RealFS{}, args: args{dir: "/bin", binary: "tool"}, - want: filepath.Join("/bin", "tool.exe"), + want: "/bin/tool.exe", }) } From 2e3a376db08db2f35931b21914f714059adaab9f Mon Sep 17 00:00:00 2001 From: Nick Fedor Date: Sat, 28 Feb 2026 05:15:53 -0700 Subject: [PATCH 05/20] test(fs): fix cross-platform path handling in tests - Replace hardcoded Unix-style paths with filepath.Join() calls - Update test cases for DetermineBinDir and AdjustBinaryPath - Ensure Windows compatibility for path separators --- internal/fs/fs_test.go | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/internal/fs/fs_test.go b/internal/fs/fs_test.go index b892c9c..2e43018 100644 --- a/internal/fs/fs_test.go +++ b/internal/fs/fs_test.go @@ -70,8 +70,8 @@ func TestRealFS_DetermineBinDir(t *testing.T) { name: "useGoroot with GOROOT set", r: &RealFS{}, args: args{useGoroot: true}, - env: map[string]string{"GOROOT": "/go"}, - want: "/go/bin", + env: map[string]string{"GOROOT": filepath.Join("/", "go")}, + want: filepath.Join("/", "go", "bin"), wantErr: false, }, { @@ -86,16 +86,16 @@ func TestRealFS_DetermineBinDir(t *testing.T) { name: "use GOBIN", r: &RealFS{}, args: args{useGoroot: false}, - env: map[string]string{"GOBIN": "/custom/bin"}, - want: "/custom/bin", + env: map[string]string{"GOBIN": filepath.Join("/", "custom", "bin")}, + want: filepath.Join("/", "custom", "bin"), wantErr: false, }, { name: "use GOPATH/bin when GOBIN unset", r: &RealFS{}, args: args{useGoroot: false}, - env: map[string]string{"GOPATH": "/gopath", "GOBIN": ""}, - want: "/gopath/bin", + env: map[string]string{"GOPATH": filepath.Join("/", "gopath"), "GOBIN": ""}, + want: filepath.Join("/", "gopath", "bin"), wantErr: false, }, { @@ -160,8 +160,8 @@ func TestRealFS_AdjustBinaryPath(t *testing.T) { { name: "basic path", r: &RealFS{}, - args: args{dir: "/bin", binary: "tool"}, - want: "/bin/tool" + func() string { + args: args{dir: filepath.Join("/", "bin"), binary: "tool"}, + want: filepath.Join("/", "bin", "tool") + func() string { if runtime.GOOS == windowsOS { return windowsExt } @@ -172,8 +172,8 @@ func TestRealFS_AdjustBinaryPath(t *testing.T) { { name: "empty binary", r: &RealFS{}, - args: args{dir: "/bin", binary: ""}, - want: filepath.Join("/bin"), //nolint:gocritic // Single argument intentional + args: args{dir: filepath.Join("/", "bin"), binary: ""}, + want: filepath.Join("/", "bin"), //nolint:gocritic // Single argument intentional }, } if runtime.GOOS == windowsOS { @@ -185,8 +185,8 @@ func TestRealFS_AdjustBinaryPath(t *testing.T) { }{ name: "windows adds .exe", r: &RealFS{}, - args: args{dir: "/bin", binary: "tool"}, - want: "/bin/tool.exe", + args: args{dir: filepath.Join("/", "bin"), binary: "tool"}, + want: filepath.Join("/", "bin", "tool.exe"), }) } From 318c139ea06403abdbf3c1f47c88246462e99dd9 Mon Sep 17 00:00:00 2001 From: Nick Fedor Date: Sat, 28 Feb 2026 05:16:38 -0700 Subject: [PATCH 06/20] Update .gitignore Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Nick Fedor --- .gitignore | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index ca2434b..2b98e20 100644 --- a/.gitignore +++ b/.gitignore @@ -2,7 +2,7 @@ # https://github.com/github/gitignore/blob/main/community/Golang/Go.AllowList.gitignore # # Binaries for programs and plugins -./bin +/bin/ *.exe *.exe~ *.dll From 35fed4f57cfbb19d39a7dd64600e40289ad99b76 Mon Sep 17 00:00:00 2001 From: Nick Fedor Date: Sat, 28 Feb 2026 05:30:12 -0700 Subject: [PATCH 07/20] test(fs): fix cross-platform path handling - Replace filepath.Join with filepath.FromSlash for OS-agnostic paths - Ensure correct path separators on Windows and Unix systems --- internal/fs/fs_test.go | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/internal/fs/fs_test.go b/internal/fs/fs_test.go index 2e43018..c262d4d 100644 --- a/internal/fs/fs_test.go +++ b/internal/fs/fs_test.go @@ -70,8 +70,8 @@ func TestRealFS_DetermineBinDir(t *testing.T) { name: "useGoroot with GOROOT set", r: &RealFS{}, args: args{useGoroot: true}, - env: map[string]string{"GOROOT": filepath.Join("/", "go")}, - want: filepath.Join("/", "go", "bin"), + env: map[string]string{"GOROOT": filepath.FromSlash("/go")}, + want: filepath.FromSlash("/go/bin"), wantErr: false, }, { @@ -86,16 +86,16 @@ func TestRealFS_DetermineBinDir(t *testing.T) { name: "use GOBIN", r: &RealFS{}, args: args{useGoroot: false}, - env: map[string]string{"GOBIN": filepath.Join("/", "custom", "bin")}, - want: filepath.Join("/", "custom", "bin"), + env: map[string]string{"GOBIN": filepath.FromSlash("/custom/bin")}, + want: filepath.FromSlash("/custom/bin"), wantErr: false, }, { name: "use GOPATH/bin when GOBIN unset", r: &RealFS{}, args: args{useGoroot: false}, - env: map[string]string{"GOPATH": filepath.Join("/", "gopath"), "GOBIN": ""}, - want: filepath.Join("/", "gopath", "bin"), + env: map[string]string{"GOPATH": filepath.FromSlash("/gopath"), "GOBIN": ""}, + want: filepath.FromSlash("/gopath/bin"), wantErr: false, }, { @@ -160,8 +160,8 @@ func TestRealFS_AdjustBinaryPath(t *testing.T) { { name: "basic path", r: &RealFS{}, - args: args{dir: filepath.Join("/", "bin"), binary: "tool"}, - want: filepath.Join("/", "bin", "tool") + func() string { + args: args{dir: filepath.FromSlash("/bin"), binary: "tool"}, + want: filepath.FromSlash("/bin/tool") + func() string { if runtime.GOOS == windowsOS { return windowsExt } @@ -172,8 +172,8 @@ func TestRealFS_AdjustBinaryPath(t *testing.T) { { name: "empty binary", r: &RealFS{}, - args: args{dir: filepath.Join("/", "bin"), binary: ""}, - want: filepath.Join("/", "bin"), //nolint:gocritic // Single argument intentional + args: args{dir: filepath.FromSlash("/bin"), binary: ""}, + want: filepath.FromSlash("/bin"), }, } if runtime.GOOS == windowsOS { @@ -185,8 +185,8 @@ func TestRealFS_AdjustBinaryPath(t *testing.T) { }{ name: "windows adds .exe", r: &RealFS{}, - args: args{dir: filepath.Join("/", "bin"), binary: "tool"}, - want: filepath.Join("/", "bin", "tool.exe"), + args: args{dir: filepath.FromSlash("/bin"), binary: "tool"}, + want: filepath.FromSlash("/bin/tool.exe"), }) } From 07500279b747db63c5222ccae43a339a7d387ea1 Mon Sep 17 00:00:00 2001 From: Nick Fedor Date: Sat, 28 Feb 2026 07:37:10 -0700 Subject: [PATCH 08/20] refactor(logger): migrate from zap to zerolog - Replace zap logger with zerolog for improved performance and simpler API - Update logger interface to use level-specific methods instead of SugaredLogger - Add SetCaptureFunc and Level methods to support test log capture and runtime level changes - Simplify root command initialization by removing redundant logger setup - Update all mocks and tests to align with the new zerolog-based interface --- cmd/root.go | 87 ++++---- cmd/root_test.go | 7 - go.mod | 5 +- go.sum | 22 +- internal/cli/cli.go | 4 +- internal/cli/cli_test.go | 223 +++++++++++++------- internal/cli/tui.go | 182 ++++++++++++++-- internal/cli/tui_test.go | 76 +++---- internal/fs/fs.go | 7 +- internal/fs/fs_test.go | 98 +++++---- internal/logger/logger.go | 322 +++++++++++++++++++++------- internal/logger/logger_test.go | 358 +++++++++++++++++++++++++++----- internal/logger/mocks/Logger.go | 251 ++++++++++++++++++++-- 13 files changed, 1270 insertions(+), 372 deletions(-) diff --git a/cmd/root.go b/cmd/root.go index 9c41f67..ff2f4ef 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -24,34 +24,20 @@ import ( "os" "github.com/spf13/cobra" - "go.uber.org/zap" - "go.uber.org/zap/zapcore" "github.com/nicholas-fedor/go-remove/internal/cli" "github.com/nicholas-fedor/go-remove/internal/fs" "github.com/nicholas-fedor/go-remove/internal/logger" ) -// ErrInvalidLoggerType indicates that the logger is not of the expected *ZapLogger type. -var ErrInvalidLoggerType = errors.New("logger is not a *ZapLogger") +// ErrInvalidLoggerType indicates that the logger is not of the expected *ZerologLogger type. +var ErrInvalidLoggerType = errors.New("logger is not a *ZerologLogger") // rootCmd defines the root command for go-remove. var rootCmd = &cobra.Command{ Use: "go-remove [binary]", Short: "A tool to remove Go binaries", RunE: func(cmd *cobra.Command, args []string) error { - // Initialize the logger for application-wide logging. - log, err := logger.NewZapLogger() - if err != nil { - return fmt.Errorf("failed to initialize logger: %w", err) - } - - // Assemble dependencies with a real filesystem and the logger instance. - deps := cli.Dependencies{ - FS: fs.NewRealFS(), - Logger: log, - } - // Extract flag values to configure CLI behavior; defaults to TUI mode if no binary is given. verbose, _ := cmd.Flags().GetBool("verbose") goroot, _ := cmd.Flags().GetBool("goroot") @@ -64,54 +50,53 @@ var rootCmd = &cobra.Command{ LogLevel: logLevel, } - // Set log level based on config if verbose mode is enabled. - if verbose { - zapLogger, ok := log.(*logger.ZapLogger) - if !ok { - return fmt.Errorf( - "failed to set log level: %w with type %T", - ErrInvalidLoggerType, - log, - ) - } + // If a binary name is provided as an argument, run in direct removal mode. + if len(args) > 0 { + config.Binary = args[0] - switch logLevel { - case "debug": - zapLogger.Logger = zapLogger.WithOptions( - zap.IncreaseLevel(zapcore.DebugLevel), - ) - case "warn": - zapLogger.Logger = zapLogger.WithOptions( - zap.IncreaseLevel(zapcore.WarnLevel), - ) - case "error": - zapLogger.Logger = zapLogger.WithOptions( - zap.IncreaseLevel(zapcore.ErrorLevel), - ) - default: - zapLogger.Logger = zapLogger.WithOptions( - zap.IncreaseLevel(zapcore.InfoLevel), - ) + // Initialize the standard logger for direct removal mode. + log, err := logger.NewLogger() + if err != nil { + return fmt.Errorf("failed to initialize logger: %w", err) } - log = zapLogger // Update the logger in deps - deps.Logger = log - } + // Set log level based on config if verbose mode is enabled. + if verbose { + level := logger.ParseLevel(logLevel) + log.Level(level) + } - // If a binary name is provided as an argument, run in direct removal mode. - if len(args) > 0 { - config.Binary = args[0] + // Assemble dependencies with a real filesystem and the logger instance. + deps := cli.Dependencies{ + FS: fs.NewRealFS(), + Logger: log, + } return cli.Run(deps, config) } // Otherwise, determine the binary directory and launch the TUI for interactive selection. - binDir, err := deps.FS.DetermineBinDir(config.Goroot) + // For TUI mode, we use a logger with capture support to display logs within the interface. + filesystem := fs.NewRealFS() + + binDir, err := filesystem.DetermineBinDir(config.Goroot) if err != nil { return fmt.Errorf("failed to determine binary directory: %w", err) } - return cli.RunTUI(binDir, config, deps.Logger, deps.FS, cli.DefaultRunner{}) + // Initialize the logger with capture support for TUI mode. + log, _, err := logger.NewLoggerWithCapture() + if err != nil { + return fmt.Errorf("failed to initialize logger: %w", err) + } + + // Set log level based on config if verbose mode is enabled. + if verbose { + level := logger.ParseLevel(logLevel) + log.Level(level) + } + + return cli.RunTUI(binDir, config, log, filesystem, cli.DefaultRunner{}) }, } diff --git a/cmd/root_test.go b/cmd/root_test.go index 600eb65..dbefd4b 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -21,8 +21,6 @@ import ( "bytes" "os" "testing" - - logmocks "github.com/nicholas-fedor/go-remove/internal/logger/mocks" ) // TestRootCommand verifies the behavior of the root command. @@ -42,9 +40,6 @@ func TestRootCommand(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - // Mock logger without Sync expectation for help flag test. - log := logmocks.NewMockLogger(t) - // Redirect stderr to capture output. oldStderr := os.Stderr r, w, _ := os.Pipe() @@ -77,8 +72,6 @@ func TestRootCommand(t *testing.T) { if gotStderr != tt.wantStderr { t.Errorf("rootCmd.Execute() stderr = %q, want %q", gotStderr, tt.wantStderr) } - - log.AssertExpectations(t) }) } } diff --git a/go.mod b/go.mod index ef1bd22..be103c2 100644 --- a/go.mod +++ b/go.mod @@ -5,9 +5,9 @@ go 1.26.0 require ( charm.land/bubbletea/v2 v2.0.0 charm.land/lipgloss/v2 v2.0.0 + github.com/rs/zerolog v1.34.0 github.com/spf13/cobra v1.10.2 github.com/stretchr/testify v1.11.1 - go.uber.org/zap v1.27.1 ) require ( @@ -22,6 +22,8 @@ require ( github.com/davecgh/go-spew v1.1.1 // indirect github.com/inconshreveable/mousetrap v1.1.0 // indirect github.com/lucasb-eyer/go-colorful v1.3.0 // indirect + github.com/mattn/go-colorable v0.1.14 // indirect + github.com/mattn/go-isatty v0.0.20 // indirect github.com/mattn/go-runewidth v0.0.20 // indirect github.com/muesli/cancelreader v0.2.2 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect @@ -29,7 +31,6 @@ require ( github.com/spf13/pflag v1.0.10 // indirect github.com/stretchr/objx v0.5.3 // indirect github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e // indirect - go.uber.org/multierr v1.11.0 // indirect golang.org/x/sync v0.19.0 // indirect golang.org/x/sys v0.41.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect diff --git a/go.sum b/go.sum index 667e38c..a6c6ca6 100644 --- a/go.sum +++ b/go.sum @@ -22,21 +22,34 @@ github.com/clipperhouse/displaywidth v0.11.0 h1:lBc6kY44VFw+TDx4I8opi/EtL9m20WSE github.com/clipperhouse/displaywidth v0.11.0/go.mod h1:bkrFNkf81G8HyVqmKGxsPufD3JhNl3dSqnGhOoSD/o0= github.com/clipperhouse/uax29/v2 v2.7.0 h1:+gs4oBZ2gPfVrKPthwbMzWZDaAFPGYK72F0NJv2v7Vk= github.com/clipperhouse/uax29/v2 v2.7.0/go.mod h1:EFJ2TJMRUaplDxHKj1qAEhCtQPW2tJSwu5BF98AuoVM= +github.com/coreos/go-systemd/v22 v22.5.0/go.mod h1:Y58oyj3AT4RCenI/lSvhwexgC+NSVTIJ3seZv2GcEnc= github.com/cpuguy83/go-md2man/v2 v2.0.6/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/godbus/dbus/v5 v5.0.4/go.mod h1:xhWf0FNVPg57R7Z0UbKHbJfkEywrmjJnf7w5xrFpKfA= github.com/inconshreveable/mousetrap v1.1.0 h1:wN+x4NVGpMsO7ErUn/mUI3vEoE6Jt13X2s0bqwp9tc8= github.com/inconshreveable/mousetrap v1.1.0/go.mod h1:vpF70FUmC8bwa3OWnCshd2FqLfsEA9PFc4w1p2J65bw= github.com/lucasb-eyer/go-colorful v1.3.0 h1:2/yBRLdWBZKrf7gB40FoiKfAWYQ0lqNcbuQwVHXptag= github.com/lucasb-eyer/go-colorful v1.3.0/go.mod h1:R4dSotOR9KMtayYi1e77YzuveK+i7ruzyGqttikkLy0= +github.com/mattn/go-colorable v0.1.13/go.mod h1:7S9/ev0klgBDR4GtXTXX8a3vIGJpMovkB8vQcUbaXHg= +github.com/mattn/go-colorable v0.1.14 h1:9A9LHSqF/7dyVVX6g0U9cwm9pG3kP9gSzcuIPHPsaIE= +github.com/mattn/go-colorable v0.1.14/go.mod h1:6LmQG8QLFO4G5z1gPvYEzlUgJ2wF+stgPZH1UqBm1s8= +github.com/mattn/go-isatty v0.0.16/go.mod h1:kYGgaQfpe5nmfYZH+SKPsOc2e4SrIfOl2e/yFXSvRLM= +github.com/mattn/go-isatty v0.0.19/go.mod h1:W+V8PltTTMOvKvAeJH7IuucS94S2C6jfK/D7dTCTo3Y= +github.com/mattn/go-isatty v0.0.20 h1:xfD0iDuEKnDkl03q4limB+vH+GxLEtL/jb4xVJSWWEY= +github.com/mattn/go-isatty v0.0.20/go.mod h1:W+V8PltTTMOvKvAeJH7IuucS94S2C6jfK/D7dTCTo3Y= github.com/mattn/go-runewidth v0.0.20 h1:WcT52H91ZUAwy8+HUkdM3THM6gXqXuLJi9O3rjcQQaQ= github.com/mattn/go-runewidth v0.0.20/go.mod h1:XBkDxAl56ILZc9knddidhrOlY5R/pDhgLpndooCuJAs= github.com/muesli/cancelreader v0.2.2 h1:3I4Kt4BQjOR54NavqnDogx/MIoWBFa0StPA8ELUXHmA= github.com/muesli/cancelreader v0.2.2/go.mod h1:3XuTXfFS2VjM+HTLZY9Ak0l6eUKfijIfMUZ4EgX0QYo= +github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/rivo/uniseg v0.4.7 h1:WUdvkW8uEhrYfLC4ZzdpI2ztxP1I582+49Oc5Mq64VQ= github.com/rivo/uniseg v0.4.7/go.mod h1:FN3SvrM+Zdj16jyLfmOkMNblXMcoc8DfTHruCPUcx88= +github.com/rs/xid v1.6.0/go.mod h1:7XoLgs4eV+QndskICGsho+ADou8ySMSjJKDIan90Nz0= +github.com/rs/zerolog v1.34.0 h1:k43nTLIwcTVQAncfCw4KZ2VY6ukYoZaBPNOE8txlOeY= +github.com/rs/zerolog v1.34.0/go.mod h1:bJsvje4Z08ROH4Nhs5iH600c3IkWhwp44iRc54W6wYQ= github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM= github.com/spf13/cobra v1.10.2 h1:DMTTonx5m65Ic0GOoRY2c16WCbHxOOw6xxezuLaBpcU= github.com/spf13/cobra v1.10.2/go.mod h1:7C1pvHqHw5A4vrJfjNwvOdzYu0Gml16OCs2GRiTUUS4= @@ -49,17 +62,14 @@ github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e h1:JVG44RsyaB9T2KIHavMF/ppJZNG9ZpyihvCd0w101no= github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e/go.mod h1:RbqR21r5mrJuqunuUZ/Dhy/avygyECGrLceyNeo4LiM= -go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto= -go.uber.org/goleak v1.3.0/go.mod h1:CoHD4mav9JJNrW/WLlf7HGZPjdw8EucARQHekz1X6bE= -go.uber.org/multierr v1.11.0 h1:blXXJkSxSSfBVBlC76pxqeO+LN3aDfLQo+309xJstO0= -go.uber.org/multierr v1.11.0/go.mod h1:20+QtiLqy0Nd6FdQB9TLXag12DsQkrbs3htMFfDN80Y= -go.uber.org/zap v1.27.1 h1:08RqriUEv8+ArZRYSTXy1LeBScaMpVSTBhCeaZYfMYc= -go.uber.org/zap v1.27.1/go.mod h1:GB2qFLM7cTU87MWRP2mPIjqfIDnGu+VIO4V/SdhGo2E= go.yaml.in/yaml/v3 v3.0.4/go.mod h1:DhzuOOF2ATzADvBadXxruRBLzYTpT36CKvDb3+aBEFg= golang.org/x/exp v0.0.0-20231006140011-7918f672742d h1:jtJma62tbqLibJ5sFQz8bKtEM8rJBtfilJ2qTU199MI= golang.org/x/exp v0.0.0-20231006140011-7918f672742d/go.mod h1:ldy0pHrwJyGW56pPQzzkH36rKxoZW1tw7ZJpeKx+hdo= golang.org/x/sync v0.19.0 h1:vV+1eWNmZ5geRlYjzm2adRgW2/mcpevXNg50YZtPCE4= golang.org/x/sync v0.19.0/go.mod h1:9KTHXmSnoGruLpwFjVSX0lNNA75CykiMECbovNTZqGI= +golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/sys v0.12.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.41.0 h1:Ivj+2Cp/ylzLiEU89QhWblYnOE9zerudt9Ftecq2C6k= golang.org/x/sys v0.41.0/go.mod h1:OgkHotnGiDImocRcuBABYBEXf8A9a87e/uXjp9XT3ks= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 87450d0..57051e1 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -27,8 +27,8 @@ import ( "github.com/nicholas-fedor/go-remove/internal/logger" ) -// ErrInvalidLoggerType indicates that the logger is not of the expected *ZapLogger type. -var ErrInvalidLoggerType = errors.New("logger is not a *ZapLogger") +// ErrInvalidLoggerType indicates that the logger is not of the expected *ZerologLogger type. +var ErrInvalidLoggerType = errors.New("logger is not a *ZerologLogger") // Config holds command-line configuration options. type Config struct { diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index 8a6b60e..5a9d562 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -24,15 +24,13 @@ import ( "os" "testing" + "github.com/rs/zerolog" "github.com/stretchr/testify/mock" - "go.uber.org/zap" - "go.uber.org/zap/zapcore" tea "charm.land/bubbletea/v2" - fsmocks "github.com/nicholas-fedor/go-remove/internal/fs/mocks" + mockFS "github.com/nicholas-fedor/go-remove/internal/fs/mocks" "github.com/nicholas-fedor/go-remove/internal/logger" - logmocks "github.com/nicholas-fedor/go-remove/internal/logger/mocks" ) // cliMockRunner mocks the ProgramRunner interface for CLI tests. @@ -40,7 +38,7 @@ type cliMockRunner struct { runProgram func(m tea.Model, opts ...tea.ProgramOption) (*tea.Program, error) } -// RunProgram executes the mock runner’s program function. +// RunProgram executes the mock runner's program function. func (m cliMockRunner) RunProgram( model tea.Model, opts ...tea.ProgramOption, @@ -53,7 +51,26 @@ func mockNoOpRunner(tea.Model, ...tea.ProgramOption) (*tea.Program, error) { return nil, nil //nolint:nilnil // Mock no-op runner } -// TestRun verifies the Run function’s behavior under various conditions. +// mockLogger implements a simple mock logger for testing. +type mockLogger struct { + syncCalled bool + syncError error + nopLogger zerolog.Logger +} + +func (m *mockLogger) Debug() *zerolog.Event { return m.nopLogger.Debug() } +func (m *mockLogger) Info() *zerolog.Event { return m.nopLogger.Info() } +func (m *mockLogger) Warn() *zerolog.Event { return m.nopLogger.Warn() } +func (m *mockLogger) Error() *zerolog.Event { return m.nopLogger.Error() } +func (m *mockLogger) Level(_ zerolog.Level) {} +func (m *mockLogger) Sync() error { + m.syncCalled = true + + return m.syncError +} +func (m *mockLogger) SetCaptureFunc(_ logger.LogCaptureFunc) {} + +// TestRun verifies the Run function's behavior under various conditions. func TestRun(t *testing.T) { tests := []struct { name string @@ -67,20 +84,15 @@ func TestRun(t *testing.T) { name: "direct removal success", config: Config{Binary: "vhs", Verbose: false, Goroot: false}, deps: Dependencies{ - FS: func() *fsmocks.MockFS { - m := fsmocks.NewMockFS(t) + FS: func() *mockFS.MockFS { + m := mockFS.NewMockFS(t) m.On("DetermineBinDir", false).Return("/bin", nil) m.On("AdjustBinaryPath", "/bin", "vhs").Return("/bin/vhs") m.On("RemoveBinary", "/bin/vhs", "vhs", false, mock.Anything).Return(nil) return m }(), - Logger: func() *logmocks.MockLogger { - m := logmocks.NewMockLogger(t) - m.On("Sync").Return(nil) - - return m - }(), + Logger: &mockLogger{}, }, wantErr: false, wantOutput: "Successfully removed vhs\n", @@ -89,8 +101,8 @@ func TestRun(t *testing.T) { name: "direct removal failure", config: Config{Binary: "vhs", Verbose: false, Goroot: false}, deps: Dependencies{ - FS: func() *fsmocks.MockFS { - m := fsmocks.NewMockFS(t) + FS: func() *mockFS.MockFS { + m := mockFS.NewMockFS(t) m.On("DetermineBinDir", false).Return("/bin", nil) m.On("AdjustBinaryPath", "/bin", "vhs").Return("/bin/vhs") m.On("RemoveBinary", "/bin/vhs", "vhs", false, mock.Anything). @@ -98,12 +110,7 @@ func TestRun(t *testing.T) { return m }(), - Logger: func() *logmocks.MockLogger { - m := logmocks.NewMockLogger(t) - m.On("Sync").Return(nil) - - return m - }(), + Logger: &mockLogger{}, }, wantErr: true, }, @@ -111,19 +118,14 @@ func TestRun(t *testing.T) { name: "tui mode success", config: Config{Binary: "", Verbose: false, Goroot: false}, deps: Dependencies{ - FS: func() *fsmocks.MockFS { - m := fsmocks.NewMockFS(t) + FS: func() *mockFS.MockFS { + m := mockFS.NewMockFS(t) m.On("DetermineBinDir", false).Return("/bin", nil) m.On("ListBinaries", "/bin").Return([]string{"vhs"}) return m }(), - Logger: func() *logmocks.MockLogger { - m := logmocks.NewMockLogger(t) - m.On("Sync").Return(nil) - - return m - }(), + Logger: &mockLogger{}, }, runner: &cliMockRunner{runProgram: mockNoOpRunner}, wantErr: false, @@ -132,19 +134,14 @@ func TestRun(t *testing.T) { name: "tui mode no binaries", config: Config{Binary: "", Verbose: false, Goroot: false}, deps: Dependencies{ - FS: func() *fsmocks.MockFS { - m := fsmocks.NewMockFS(t) + FS: func() *mockFS.MockFS { + m := mockFS.NewMockFS(t) m.On("DetermineBinDir", false).Return("/bin", nil) m.On("ListBinaries", "/bin").Return([]string{}) return m }(), - Logger: func() *logmocks.MockLogger { - m := logmocks.NewMockLogger(t) - m.On("Sync").Return(nil) - - return m - }(), + Logger: &mockLogger{}, }, runner: &cliMockRunner{runProgram: mockNoOpRunner}, wantErr: true, @@ -153,18 +150,13 @@ func TestRun(t *testing.T) { name: "bin dir error", config: Config{Binary: "vhs", Verbose: false, Goroot: false}, deps: Dependencies{ - FS: func() *fsmocks.MockFS { - m := fsmocks.NewMockFS(t) + FS: func() *mockFS.MockFS { + m := mockFS.NewMockFS(t) m.On("DetermineBinDir", false).Return("", errors.New("bin dir failed")) return m }(), - Logger: func() *logmocks.MockLogger { - m := logmocks.NewMockLogger(t) - m.On("Sync").Return(nil) - - return m - }(), + Logger: &mockLogger{}, }, wantErr: true, }, @@ -172,20 +164,15 @@ func TestRun(t *testing.T) { name: "logger sync error", config: Config{Binary: "vhs", Verbose: false, Goroot: false}, deps: Dependencies{ - FS: func() *fsmocks.MockFS { - m := fsmocks.NewMockFS(t) + FS: func() *mockFS.MockFS { + m := mockFS.NewMockFS(t) m.On("DetermineBinDir", false).Return("/bin", nil) m.On("AdjustBinaryPath", "/bin", "vhs").Return("/bin/vhs") m.On("RemoveBinary", "/bin/vhs", "vhs", false, mock.Anything).Return(nil) return m }(), - Logger: func() *logmocks.MockLogger { - m := logmocks.NewMockLogger(t) - m.On("Sync").Return(errors.New("sync failed")) - - return m - }(), + Logger: &mockLogger{syncError: errors.New("sync failed")}, }, wantErr: false, // Sync errors are ignored on all platforms wantOutput: "Successfully removed vhs\n", @@ -211,7 +198,7 @@ func TestRun(t *testing.T) { // Set log level based on config if verbose mode is enabled. if config.Verbose { - zapLogger, ok := log.(*logger.ZapLogger) + zl, ok := log.(*logger.ZerologLogger) if !ok { return fmt.Errorf( "failed to set log level: %w with type %T", @@ -220,26 +207,8 @@ func TestRun(t *testing.T) { ) } - switch config.LogLevel { - case "debug": - zapLogger.Logger = zapLogger.WithOptions( - zap.IncreaseLevel(zapcore.DebugLevel), - ) - case "warn": - zapLogger.Logger = zapLogger.WithOptions( - zap.IncreaseLevel(zapcore.WarnLevel), - ) - case "error": - zapLogger.Logger = zapLogger.WithOptions( - zap.IncreaseLevel(zapcore.ErrorLevel), - ) - default: - zapLogger.Logger = zapLogger.WithOptions( - zap.IncreaseLevel(zapcore.InfoLevel), - ) - } - - log = zapLogger + level := logger.ParseLevel(config.LogLevel) + zl.Level(level) } binDir, err := deps.FS.DetermineBinDir(config.Goroot) @@ -298,8 +267,108 @@ func TestRun(t *testing.T) { } // Assert that all mock expectations were met. - tt.deps.FS.(*fsmocks.MockFS).AssertExpectations(t) - tt.deps.Logger.(*logmocks.MockLogger).AssertExpectations(t) + tt.deps.FS.(*mockFS.MockFS).AssertExpectations(t) }) } } + +// TestRun_WithLoggerSync verifies that Sync is called appropriately. +func TestRun_WithLoggerSync(t *testing.T) { + tests := []struct { + name string + config Config + setupFS func() *mockFS.MockFS + expectSyncCall bool + }{ + { + name: "sync called on success", + config: Config{Binary: "tool", Verbose: false, Goroot: false}, + setupFS: func() *mockFS.MockFS { + m := mockFS.NewMockFS(t) + m.On("DetermineBinDir", false).Return("/bin", nil) + m.On("AdjustBinaryPath", "/bin", "tool").Return("/bin/tool") + m.On("RemoveBinary", "/bin/tool", "tool", false, mock.Anything).Return(nil) + + return m + }, + expectSyncCall: true, + }, + { + name: "sync called on error", + config: Config{Binary: "tool", Verbose: false, Goroot: false}, + setupFS: func() *mockFS.MockFS { + m := mockFS.NewMockFS(t) + m.On("DetermineBinDir", false).Return("", errors.New("bin dir error")) + + return m + }, + expectSyncCall: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mockLog := &mockLogger{} + deps := Dependencies{ + FS: tt.setupFS(), + Logger: mockLog, + } + + binDir, err := deps.FS.DetermineBinDir(tt.config.Goroot) + if err != nil { + _ = deps.Logger.Sync() + + return + } + + binaryPath := deps.FS.AdjustBinaryPath(binDir, tt.config.Binary) + _ = deps.FS.RemoveBinary(binaryPath, tt.config.Binary, tt.config.Verbose, deps.Logger) + _ = deps.Logger.Sync() + + if tt.expectSyncCall && !mockLog.syncCalled { + t.Errorf("Expected Sync() to be called") + } + }) + } +} + +// TestRun_VerboseMode verifies verbose mode behavior. +func TestRun_VerboseMode(t *testing.T) { + m := mockFS.NewMockFS(t) + m.On("DetermineBinDir", false).Return("/bin", nil) + m.On("AdjustBinaryPath", "/bin", "vhs").Return("/bin/vhs") + m.On("RemoveBinary", "/bin/vhs", "vhs", true, mock.Anything).Return(nil) + + mockLog := &mockLogger{} + deps := Dependencies{ + FS: m, + Logger: mockLog, + } + config := Config{Binary: "vhs", Verbose: true, Goroot: false} + + // Redirect stdout to capture output. + oldStdout := os.Stdout + r, w, _ := os.Pipe() + os.Stdout = w + + binDir, _ := deps.FS.DetermineBinDir(config.Goroot) + binaryPath := deps.FS.AdjustBinaryPath(binDir, config.Binary) + err := deps.FS.RemoveBinary(binaryPath, config.Binary, config.Verbose, deps.Logger) + _ = deps.Logger.Sync() + + w.Close() + + os.Stdout = oldStdout + + var buf bytes.Buffer + buf.ReadFrom(r) + + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + + // In verbose mode, no success message should be printed to stdout. + if buf.String() != "" { + t.Errorf("Expected no stdout output in verbose mode, got: %q", buf.String()) + } +} diff --git a/internal/cli/tui.go b/internal/cli/tui.go index 02bb09b..0faa853 100644 --- a/internal/cli/tui.go +++ b/internal/cli/tui.go @@ -34,17 +34,26 @@ import ( // Layout constants for TUI rendering. const ( - colWidthPadding = 3 // Padding added to column width for spacing - availWidthAdjustment = 4 // Adjustment to width for border and padding - minAvailHeightAdjustment = 7 // Minimum height adjustment for UI elements - visibleLenPrefix = 2 // Prefix length for cursor visibility - totalHeightBase = 4 // Base height for non-grid UI components - leftPadding = 2 // Left padding for the entire TUI + colWidthPadding = 3 // Padding added to column width for spacing + availWidthAdjustment = 4 // Adjustment to width for border and padding + minAvailHeightAdjustment = 7 // Minimum height adjustment for UI elements + visibleLenPrefix = 2 // Prefix length for cursor visibility + totalHeightBase = 4 // Base height for non-grid UI components + leftPadding = 2 // Left padding for the entire TUI + maxLogLines = 50 // Maximum number of log lines to retain + maxVisibleLogLines = 5 // Maximum number of log lines to display + logPanelSeparatorLines = 2 // Number of separator lines for log panel ) // ErrNoBinariesFound signals that no binaries were found in the target directory. var ErrNoBinariesFound = errors.New("no binaries found in directory") +// LogMsg is a Bubble Tea message that carries a log entry to be displayed in the TUI. +type LogMsg struct { + Level string // Log level (e.g., "DBG", "INF", "WRN", "ERR") + Message string // Log message content +} + // ProgramRunner defines an interface for running Bubbletea programs. type ProgramRunner interface { RunProgram(m tea.Model, opts ...tea.ProgramOption) (*tea.Program, error) @@ -56,6 +65,7 @@ type styleConfig struct { CursorColor string // ANSI 256-color code for cursor FooterColor string // ANSI 256-color code for footer StatusColor string // ANSI 256-color code for status + LogColor string // ANSI 256-color code for log messages Cursor string // Symbol used for the cursor } @@ -75,11 +85,20 @@ type model struct { status string // Status message styles styleConfig // TUI appearance settings sortAscending bool // True for ascending sort, false for descending + logs []string // Captured log messages (circular buffer) + showLogs bool // Toggle log panel visibility + program *tea.Program // Reference to the Bubble Tea program for sending messages + logChan chan LogMsg // Channel for receiving log messages from the logger } // DefaultRunner provides the default Bubbletea program runner. type DefaultRunner struct{} +// NewLogMsg creates a new LogMsg for sending log entries to the TUI. +func NewLogMsg(level, message string) LogMsg { + return LogMsg{Level: level, Message: message} +} + // RunTUI launches the interactive TUI mode for binary selection and removal. func RunTUI( dir string, @@ -94,8 +113,9 @@ func RunTUI( return fmt.Errorf("%w: %s", ErrNoBinariesFound, dir) } - // Initialize and start the TUI program with default styles. - program, err := runner.RunProgram(&model{ + // Initialize the model with default styles. + // Enable log visibility by default when verbose mode is active. + m := &model{ choices: choices, dir: dir, config: config, @@ -105,7 +125,28 @@ func RunTUI( cursorY: 0, sortAscending: true, styles: defaultStyleConfig(), - }) + logs: make([]string, 0, maxLogLines), + showLogs: config.Verbose, + } + + // Set up log capture channel if verbose mode is enabled. + // This must be done before starting the program to ensure capture is ready. + if config.Verbose { + m.logChan = make(chan LogMsg, maxLogLines) + + log.SetCaptureFunc(func(level, msg string) { + // Send to channel without blocking. + // If channel is full, the message is dropped to prevent blocking. + select { + case m.logChan <- NewLogMsg(level, msg): + default: + // Channel is full, message is dropped to prevent blocking. + } + }) + } + + // Start the TUI program. + program, err := runner.RunProgram(m) if err != nil { return fmt.Errorf("failed to start TUI program: %w", err) } @@ -115,6 +156,9 @@ func RunTUI( return nil } + // Store program reference for log message sending. + m.program = program + // Run the program and capture any runtime errors. _, err = program.Run() if err != nil { @@ -131,6 +175,7 @@ func defaultStyleConfig() styleConfig { CursorColor: "214", // Orange FooterColor: "245", // Light gray StatusColor: "46", // Lime green + LogColor: "240", // Dark gray for subtle log display Cursor: "❯ ", } } @@ -146,9 +191,31 @@ func (r DefaultRunner) RunProgram(m tea.Model, opts ...tea.ProgramOption) (*tea. func (m *model) Init() tea.Cmd { m.sortChoices() + // Start log polling if verbose mode is enabled. + if m.config.Verbose && m.logChan != nil { + return m.pollLogChannel() + } + return nil } +// pollLogChannel returns a command that polls for log messages. +// This approach avoids deadlocks by having the TUI poll the channel +// rather than trying to Send() from within the capture callback. +func (m *model) pollLogChannel() tea.Cmd { + return func() tea.Msg { + // Check if there's a message on the channel. + // Use a non-blocking receive to avoid hanging. + select { + case msg := <-m.logChan: + return msg + default: + // No message available, schedule next poll. + return nil + } + } +} + // Update processes TUI events and updates the model state. func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { switch msg := msg.(type) { @@ -194,6 +261,10 @@ func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.sortChoices() m.updateGrid() + case "L": + // Toggle log panel visibility. + m.showLogs = !m.showLogs + case "enter": // Remove the selected binary and update the TUI state. if len(m.choices) > 0 { @@ -236,11 +307,57 @@ func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.width = msg.Width m.height = msg.Height m.updateGrid() + case LogMsg: + // Add log message to the circular buffer. + m.addLogEntry(msg) + + // Continue polling for more log messages. + // This ensures all pending logs are captured. + cmd := m.pollLogChannel() + + return m, cmd + } + + // Continue polling for log messages if in verbose mode. + // This non-blocking poll ensures logs are captured without deadlocks. + if m.config.Verbose && m.logChan != nil { + cmd := m.pollLogChannel() + + return m, cmd } return m, nil } +// addLogEntry adds a log message to the circular buffer, maintaining maxLogLines limit. +func (m *model) addLogEntry(msg LogMsg) { + // Format the log entry: "[LEVEL] message" + entry := fmt.Sprintf("[%s] %s", msg.Level, msg.Message) + + // Add to logs slice + m.logs = append(m.logs, entry) + + // Maintain circular buffer size + if len(m.logs) > maxLogLines { + m.logs = m.logs[len(m.logs)-maxLogLines:] + } +} + +// getVisibleLogs returns the last N log lines that fit within the available height. +func (m *model) getVisibleLogs(maxLines int) []string { + if !m.showLogs || len(m.logs) == 0 || maxLines <= 0 { + return nil + } + + // Return the last maxLines entries + start := len(m.logs) - maxLines + if start < 0 { + start = 0 + } + + return m.logs[start:] +} + // sortChoices sorts the choices based on the current sort order. func (m *model) sortChoices() { if len(m.choices) == 0 { @@ -269,6 +386,13 @@ func (m *model) updateGrid() { availWidth := m.width - availWidthAdjustment availHeight := maximum(m.height-minAvailHeightAdjustment, 1) + // Adjust available height for log panel if visible + if m.showLogs && len(m.logs) > 0 { + // Reserve up to maxVisibleLogLines lines for log panel (plus 1 for separator) + logPanelHeight := minimum(len(m.logs), maxVisibleLogLines) + 1 + availHeight = maximum(availHeight-logPanelHeight, 1) + } + // Clear grid if no choices remain. if len(m.choices) == 0 { m.rows = 0 @@ -320,6 +444,7 @@ func (m *model) View() tea.View { cursorStyle := lipgloss.NewStyle().Foreground(lipgloss.Color(m.styles.CursorColor)) footerStyle := lipgloss.NewStyle().Foreground(lipgloss.Color(m.styles.FooterColor)) statusStyle := lipgloss.NewStyle().Foreground(lipgloss.Color(m.styles.StatusColor)) + logStyle := lipgloss.NewStyle().Foreground(lipgloss.Color(m.styles.LogColor)) // Calculate column width based on the longest binary name. var maxNameLen int @@ -356,7 +481,7 @@ func (m *model) View() tea.View { grid.WriteString("\n") } - // Assemble the full TUI layout: title, grid, status, and footer. + // Assemble the full TUI layout: title, grid, logs (if visible), status, and footer. var s strings.Builder s.WriteString(titleStyle.Render("Select a binary to remove:\n")) @@ -364,21 +489,50 @@ func (m *model) View() tea.View { s.WriteString(grid.String()) s.WriteString("\n") + // Render log panel if enabled and logs exist + if m.showLogs && len(m.logs) > 0 { + visibleLogs := m.getVisibleLogs( + maxVisibleLogLines, + ) // Show up to maxVisibleLogLines log lines + if len(visibleLogs) > 0 { + s.WriteString(logStyle.Render("─ Log Messages ─")) + s.WriteString("\n") + + for _, logEntry := range visibleLogs { + s.WriteString(logStyle.Render(logEntry)) + s.WriteString("\n") + } + + s.WriteString("\n") + } + } + if m.status != "" { s.WriteString(statusStyle.Render(m.status)) s.WriteString("\n") } - footer := footerStyle.Render( - "↑/k: up ↓/j: down ←/h: left →/l: right Enter: remove s: toggle sort q: quit", - ) + // Update footer to include L key for toggling logs + footerText := "↑/k: up ↓/j: down ←/h: left →/l: right Enter: remove s: toggle sort L: logs q: quit" + footer := footerStyle.Render(footerText) lenStatus := 0 if m.status != "" { lenStatus = 1 } - totalHeight := m.rows + totalHeightBase + lenStatus + // Account for log panel in height calculation + logPanelLines := 0 + + if m.showLogs && len(m.logs) > 0 { + visibleLogs := m.getVisibleLogs(maxVisibleLogLines) + if len(visibleLogs) > 0 { + // Log panel header + log lines + separator + logPanelLines = len(visibleLogs) + logPanelSeparatorLines + } + } + + totalHeight := m.rows + totalHeightBase + lenStatus + logPanelLines // Add padding lines to fill the terminal height. for i := totalHeight; i < m.height; i++ { diff --git a/internal/cli/tui_test.go b/internal/cli/tui_test.go index d8edf4e..d304f5e 100644 --- a/internal/cli/tui_test.go +++ b/internal/cli/tui_test.go @@ -25,14 +25,28 @@ import ( "testing" "unicode/utf8" + "github.com/rs/zerolog" "github.com/stretchr/testify/mock" tea "charm.land/bubbletea/v2" - fsmocks "github.com/nicholas-fedor/go-remove/internal/fs/mocks" - logmocks "github.com/nicholas-fedor/go-remove/internal/logger/mocks" + mockFS "github.com/nicholas-fedor/go-remove/internal/fs/mocks" + "github.com/nicholas-fedor/go-remove/internal/logger" ) +// tuiMockLogger is a simple mock logger for TUI tests. +type tuiMockLogger struct { + nopLogger zerolog.Logger +} + +func (m *tuiMockLogger) Debug() *zerolog.Event { return m.nopLogger.Debug() } +func (m *tuiMockLogger) Info() *zerolog.Event { return m.nopLogger.Info() } +func (m *tuiMockLogger) Warn() *zerolog.Event { return m.nopLogger.Warn() } +func (m *tuiMockLogger) Error() *zerolog.Event { return m.nopLogger.Error() } +func (m *tuiMockLogger) Sync() error { return nil } +func (m *tuiMockLogger) Level(_ zerolog.Level) {} +func (m *tuiMockLogger) SetCaptureFunc(_ logger.LogCaptureFunc) {} + // tuiMockRunner mocks the ProgramRunner interface for TUI tests. type tuiMockRunner struct { runProgram func(tea.Model, ...tea.ProgramOption) (*tea.Program, error) @@ -51,8 +65,8 @@ func TestRunTUI(t *testing.T) { type args struct { dir string config Config - logger *logmocks.MockLogger - fs *fsmocks.MockFS + logger logger.Logger + fs *mockFS.MockFS runner ProgramRunner } @@ -66,9 +80,9 @@ func TestRunTUI(t *testing.T) { args: args{ dir: "/bin", config: Config{}, - logger: logmocks.NewMockLogger(t), - fs: func() *fsmocks.MockFS { - m := fsmocks.NewMockFS(t) + logger: &tuiMockLogger{}, + fs: func() *mockFS.MockFS { + m := mockFS.NewMockFS(t) m.On("ListBinaries", "/bin").Return([]string{"vhs"}) return m @@ -82,9 +96,9 @@ func TestRunTUI(t *testing.T) { args: args{ dir: "/bin", config: Config{}, - logger: logmocks.NewMockLogger(t), - fs: func() *fsmocks.MockFS { - m := fsmocks.NewMockFS(t) + logger: &tuiMockLogger{}, + fs: func() *mockFS.MockFS { + m := mockFS.NewMockFS(t) m.On("ListBinaries", "/bin").Return([]string{}) return m @@ -98,9 +112,9 @@ func TestRunTUI(t *testing.T) { args: args{ dir: "/bin", config: Config{}, - logger: logmocks.NewMockLogger(t), - fs: func() *fsmocks.MockFS { - m := fsmocks.NewMockFS(t) + logger: &tuiMockLogger{}, + fs: func() *mockFS.MockFS { + m := mockFS.NewMockFS(t) m.On("ListBinaries", "/bin").Return([]string{"vhs"}) return m @@ -123,7 +137,6 @@ func TestRunTUI(t *testing.T) { } tt.args.fs.AssertExpectations(t) - tt.args.logger.AssertExpectations(t) }) } } @@ -301,13 +314,9 @@ func Test_model_Update(t *testing.T) { sortAscending: true, width: 80, height: 24, - logger: func() *logmocks.MockLogger { - m := logmocks.NewMockLogger(t) - - return m - }(), - fs: func() *fsmocks.MockFS { - m := fsmocks.NewMockFS(t) + logger: &tuiMockLogger{}, + fs: func() *mockFS.MockFS { + m := mockFS.NewMockFS(t) m.On("AdjustBinaryPath", "/bin", "age").Return("/bin/age") m.On("RemoveBinary", "/bin/age", "age", false, mock.Anything).Return(nil) m.On("ListBinaries", "/bin").Return([]string{"vhs"}) @@ -339,13 +348,9 @@ func Test_model_Update(t *testing.T) { sortAscending: true, width: 80, height: 24, - logger: func() *logmocks.MockLogger { - m := logmocks.NewMockLogger(t) - - return m - }(), - fs: func() *fsmocks.MockFS { - m := fsmocks.NewMockFS(t) + logger: &tuiMockLogger{}, + fs: func() *mockFS.MockFS { + m := mockFS.NewMockFS(t) m.On("AdjustBinaryPath", "/bin", "age").Return("/bin/age") m.On("RemoveBinary", "/bin/age", "age", false, mock.Anything). Return(errors.New("remove failed")) @@ -385,11 +390,11 @@ func Test_model_Update(t *testing.T) { t.Run(tt.name, func(t *testing.T) { // Set default mocks if not provided. if tt.m.logger == nil { - tt.m.logger = logmocks.NewMockLogger(t) + tt.m.logger = &tuiMockLogger{} } if tt.m.fs == nil { - tt.m.fs = fsmocks.NewMockFS(t) + tt.m.fs = mockFS.NewMockFS(t) } got, gotCmd := tt.m.Update(tt.args.msg) @@ -413,8 +418,7 @@ func Test_model_Update(t *testing.T) { t.Errorf("model.Update() gotCmd = %T, want %T", gotCmd, tt.wantCmd) } - tt.m.fs.(*fsmocks.MockFS).AssertExpectations(t) - tt.m.logger.(*logmocks.MockLogger).AssertExpectations(t) + tt.m.fs.(*mockFS.MockFS).AssertExpectations(t) }) } } @@ -554,8 +558,8 @@ func Test_model_View(t *testing.T) { lines = append(lines, leftPaddingStr+pad("", effectiveWidth)) } - footerPart1 := "↑/k: up ↓/j: down ←/h: left →/l: right Enter: remove s: toggle sort q:" - footerPart2 := "quit" + footerPart1 := "↑/k: up ↓/j: down ←/h: left →/l: right Enter: remove s: toggle sort L:" + footerPart2 := "logs q: quit" lines = append( lines, @@ -596,8 +600,8 @@ func Test_model_View(t *testing.T) { lines = append(lines, leftPaddingStr+pad("", effectiveWidth)) } - footerPart1 := "↑/k: up ↓/j: down ←/h: left →/l: right Enter: remove s: toggle sort q:" - footerPart2 := "quit" + footerPart1 := "↑/k: up ↓/j: down ←/h: left →/l: right Enter: remove s: toggle sort L:" + footerPart2 := "logs q: quit" lines = append( lines, diff --git a/internal/fs/fs.go b/internal/fs/fs.go index 82274b3..0c7e12c 100644 --- a/internal/fs/fs.go +++ b/internal/fs/fs.go @@ -109,10 +109,9 @@ func (r *RealFS) RemoveBinary(binaryPath, name string, verbose bool, log logger. } // Log debug and info messages if verbose mode is enabled. - sugar := log.Sugar() if verbose { - sugar.Debugf("Constructed binary path: %s", binaryPath) - sugar.Infof("Removing binary: %s", binaryPath) + log.Debug().Msgf("Constructed binary path: %s", binaryPath) + log.Info().Msgf("Removing binary: %s", binaryPath) } // Perform the removal operation and handle any errors. @@ -122,7 +121,7 @@ func (r *RealFS) RemoveBinary(binaryPath, name string, verbose bool, log logger. // Log success if verbose mode is enabled. if verbose { - sugar.Infof("Successfully removed binary: %s", name) + log.Info().Msgf("Successfully removed binary: %s", name) } return nil diff --git a/internal/fs/fs_test.go b/internal/fs/fs_test.go index c262d4d..0c3e48e 100644 --- a/internal/fs/fs_test.go +++ b/internal/fs/fs_test.go @@ -25,13 +25,20 @@ import ( "sort" "testing" - "go.uber.org/zap" + "github.com/rs/zerolog" "github.com/nicholas-fedor/go-remove/internal/logger" - logmocks "github.com/nicholas-fedor/go-remove/internal/logger/mocks" + "github.com/nicholas-fedor/go-remove/internal/logger/mocks" ) -// TestNewRealFS verifies the NewRealFS function’s instance creation. +// nopLogger creates a mock logger for testing. +func nopLogger(t *testing.T) logger.Logger { + t.Helper() + + return mocks.NewMockLogger(t) +} + +// TestNewRealFS verifies the NewRealFS function's instance creation. func TestNewRealFS(t *testing.T) { tests := []struct { name string @@ -52,7 +59,7 @@ func TestNewRealFS(t *testing.T) { } } -// TestRealFS_DetermineBinDir verifies the DetermineBinDir method’s directory resolution. +// TestRealFS_DetermineBinDir verifies the DetermineBinDir method's directory resolution. func TestRealFS_DetermineBinDir(t *testing.T) { type args struct { useGoroot bool @@ -118,7 +125,7 @@ func TestRealFS_DetermineBinDir(t *testing.T) { t.Setenv(k, v) } - // Adjust expected path for the platform’s HOME directory. + // Adjust expected path for the platform's HOME directory. if tt.name == "use default ~/go/bin when GOBIN and GOPATH unset" { home := os.Getenv("HOME") if runtime.GOOS == windowsOS { @@ -144,7 +151,7 @@ func TestRealFS_DetermineBinDir(t *testing.T) { } } -// TestRealFS_AdjustBinaryPath verifies the AdjustBinaryPath method’s path construction. +// TestRealFS_AdjustBinaryPath verifies the AdjustBinaryPath method's path construction. func TestRealFS_AdjustBinaryPath(t *testing.T) { type args struct { dir string @@ -199,13 +206,13 @@ func TestRealFS_AdjustBinaryPath(t *testing.T) { } } -// TestRealFS_RemoveBinary verifies the RemoveBinary method’s file removal behavior. +// TestRealFS_RemoveBinary verifies the RemoveBinary method's file removal behavior. func TestRealFS_RemoveBinary(t *testing.T) { type args struct { binaryPath string name string verbose bool - logger logger.Logger + logger func() logger.Logger // Factory function to create logger } tests := []struct { @@ -221,12 +228,9 @@ func TestRealFS_RemoveBinary(t *testing.T) { args: args{ name: "testbin", verbose: false, - logger: func() *logmocks.MockLogger { - m := logmocks.NewMockLogger(t) - m.On("Sugar").Return(zap.NewNop().Sugar()).Maybe() - - return m - }(), + logger: func() logger.Logger { + return nopLogger(t) + }, }, setup: func() string { tmpDir := t.TempDir() @@ -244,12 +248,9 @@ func TestRealFS_RemoveBinary(t *testing.T) { binaryPath: "/nonexistent/testbin", name: "testbin", verbose: false, - logger: func() *logmocks.MockLogger { - m := logmocks.NewMockLogger(t) - m.On("Sugar").Return(zap.NewNop().Sugar()).Maybe() - - return m - }(), + logger: func() logger.Logger { + return nopLogger(t) + }, }, wantErr: true, }, @@ -259,13 +260,16 @@ func TestRealFS_RemoveBinary(t *testing.T) { args: args{ name: "testbin", verbose: true, - logger: func() *logmocks.MockLogger { - m := logmocks.NewMockLogger(t) - // Expect Sugar() to be called at least once, without strict count - m.On("Sugar").Return(zap.NewNop().Sugar()).Maybe() - - return m - }(), + logger: func() logger.Logger { + // Create mock with expectations for verbose logging. + log := mocks.NewMockLogger(t) + zl := zerolog.New(nil).With().Logger() + dummyEvent := zl.Debug() + log.On("Debug").Return(dummyEvent) + log.On("Info").Return(dummyEvent) + + return log + }, }, setup: func() string { tmpDir := t.TempDir() @@ -289,24 +293,44 @@ func TestRealFS_RemoveBinary(t *testing.T) { tt.args.binaryPath, tt.args.name, tt.args.verbose, - tt.args.logger, + tt.args.logger(), // Call factory to get logger ) if (err != nil) != tt.wantErr { t.Errorf("RemoveBinary() error = %v, wantErr %v", err, tt.wantErr) } - - // Assert that all mock expectations were met and log detailed expectations for debugging. - if tt.args.logger != nil { - mockLogger := tt.args.logger.(*logmocks.MockLogger) - t.Logf("Mock expectations for %s: %v", tt.name, mockLogger.ExpectedCalls) - t.Logf("Mock calls made for %s: %v", tt.name, mockLogger.Calls) - mockLogger.AssertExpectations(t) - } }) } } -// TestRealFS_ListBinaries verifies the ListBinaries method’s directory listing. +// TestRealFS_RemoveBinary_VerboseLogging verifies verbose logging behavior. +func TestRealFS_RemoveBinary_VerboseLogging(t *testing.T) { + log := mocks.NewMockLogger(t) + + // Create a dummy zerolog logger and event for return values. + zl := zerolog.New(nil).With().Logger() + dummyEvent := zl.Debug() + + // Set up expectations for verbose logging calls. + log.On("Debug").Return(dummyEvent) + log.On("Info").Return(dummyEvent) + + tmpDir := t.TempDir() + tmpFile := filepath.Join(tmpDir, "testbin") + os.WriteFile(tmpFile, []byte("test"), 0o755) + + fs := &RealFS{} + + err := fs.RemoveBinary(tmpFile, "testbin", true, log) + if err != nil { + t.Errorf("RemoveBinary() unexpected error = %v", err) + } + + // Verify that Debug and Info were called. + log.AssertCalled(t, "Debug") + log.AssertCalled(t, "Info") +} + +// TestRealFS_ListBinaries verifies the ListBinaries method's directory listing. func TestRealFS_ListBinaries(t *testing.T) { type args struct { dir string diff --git a/internal/logger/logger.go b/internal/logger/logger.go index 4359904..4859508 100644 --- a/internal/logger/logger.go +++ b/internal/logger/logger.go @@ -21,99 +21,279 @@ package logger import ( "errors" "fmt" + "io" "os" + "strings" + "sync" + "time" - "go.uber.org/zap" - "go.uber.org/zap/zapcore" -) - -// Sampling constants for logger configuration. -const ( - samplingInitial = 100 // Initial number of log entries before sampling - samplingThereafter = 100 // Number of log entries to sample after initial + "github.com/rs/zerolog" ) // ErrLoggerNil indicates that the logger instance is nil when an operation is attempted. var ErrLoggerNil = errors.New("logger is nil") +// LogCaptureFunc is a callback function that receives captured log messages. +// The level parameter contains the log level string (e.g., "DBG", "INF", "WRN", "ERR"). +// The msg parameter contains the formatted log message. +type LogCaptureFunc func(level, msg string) + // Logger defines the logging operations required by the application. type Logger interface { - Sync() error // Flush any buffered log entries - Sugar() *zap.SugaredLogger // Return a sugared logger for key-value logging -} - -// ZapLogger adapts zap.Logger to implement the Logger interface. -type ZapLogger struct { - *zap.Logger // Embedded zap.Logger for core logging functionality -} - -// NewZapLogger creates a new production-ready Zap logger. -func NewZapLogger() (Logger, error) { - // Define a custom configuration for the logger to ensure cross-platform compatibility. - config := zap.Config{ - Level: zap.NewAtomicLevelAt(zapcore.InfoLevel), - Development: false, - Sampling: &zap.SamplingConfig{ - Initial: samplingInitial, - Thereafter: samplingThereafter, - }, - Encoding: "console", // Use console encoding for human-readable output - EncoderConfig: zapcore.EncoderConfig{ - TimeKey: "time", - LevelKey: "level", - NameKey: "logger", - CallerKey: "caller", - MessageKey: "msg", - StacktraceKey: "stacktrace", - LineEnding: zapcore.DefaultLineEnding, - EncodeLevel: zapcore.CapitalLevelEncoder, // Use uppercase level names (e.g., INFO) - EncodeTime: zapcore.ISO8601TimeEncoder, // Use ISO 8601 format for timestamps - EncodeDuration: zapcore.StringDurationEncoder, - EncodeCaller: zapcore.ShortCallerEncoder, - }, - OutputPaths: []string{"stderr"}, // Explicitly write to stderr - ErrorOutputPaths: []string{"stderr"}, - } + // Debug returns a debug-level event for logging. + Debug() *zerolog.Event + // Info returns an info-level event for logging. + Info() *zerolog.Event + // Warn returns a warn-level event for logging. + Warn() *zerolog.Event + // Error returns an error-level event for logging. + Error() *zerolog.Event + // Sync flushes any buffered log entries (no-op for zerolog, kept for compatibility). + Sync() error + // Level sets the minimum log level dynamically. + Level(level zerolog.Level) + // SetCaptureFunc sets a callback function to capture log messages for TUI display. + // When set, log messages are sent to this callback in addition to normal output. + SetCaptureFunc(captureFunc LogCaptureFunc) +} + +// ZerologLogger wraps zerolog.Logger to implement the Logger interface. +type ZerologLogger struct { + logger zerolog.Logger + mu sync.RWMutex + output io.Writer + captureFunc LogCaptureFunc +} - // Build the logger with a locked writer to ensure thread-safe writes to stderr. - logger, err := config.Build(zap.WrapCore(func(_ zapcore.Core) zapcore.Core { - writer := zapcore.Lock(os.Stderr) +// captureWriter wraps an io.Writer and captures written data for TUI display. +type captureWriter struct { + mu sync.RWMutex + output io.Writer + captureFunc LogCaptureFunc +} - return zapcore.NewCore( - zapcore.NewConsoleEncoder(config.EncoderConfig), - writer, - config.Level, - ) - })) +// Write implements io.Writer, writing to the underlying output and capturing the data. +// When a capture function is set, output is discarded to prevent duplicate logs in TUI mode. +func (w *captureWriter) Write(data []byte) (int, error) { + w.mu.RLock() + output := w.output + capture := w.captureFunc + w.mu.RUnlock() + + // If a capture function is set, discard the output to prevent duplicate logs. + // The log message will only be sent through the capture mechanism to the TUI log panel. + if capture != nil { + w.captureLogMessage(string(data)) + + // Write to io.Discard to satisfy the writer interface without producing output. + bytesWritten, err := io.Discard.Write(data) + if err != nil { + return bytesWritten, fmt.Errorf("failed to write to discard: %w", err) + } + + return bytesWritten, nil + } + + // No capture function set, write to the underlying output normally. + bytesWritten, err := output.Write(data) if err != nil { - return nil, fmt.Errorf("failed to initialize logger: %w", err) + return bytesWritten, fmt.Errorf("failed to write to output: %w", err) } - return &ZapLogger{logger}, nil + return bytesWritten, nil } -// Sync flushes any buffered log entries to the underlying output. -func (z *ZapLogger) Sync() error { - // Check for nil logger to avoid panics on uninitialized instances. - if z.Logger == nil { - return ErrLoggerNil +// captureLogMessage parses and captures the log message. +// Expected format from ConsoleWriter: " ". +func (w *captureWriter) captureLogMessage(logLine string) { + w.mu.RLock() + capture := w.captureFunc + w.mu.RUnlock() + + if capture == nil { + return + } + + // Parse the log line to extract level and message + // Format: "2006-01-02T15:04:05Z07:00 DBG message here" + parts := strings.Fields(logLine) + + const minLogParts = 3 // timestamp, level, message + if len(parts) < minLogParts { + // Not enough parts, use the whole line + capture("LOG", strings.TrimSpace(logLine)) + + return } - // Attempt to sync, logging any errors in debug mode but not returning them. - if err := z.Logger.Sync(); err != nil { - z.Debug("Logger sync failed", zap.Error(err)) + // The level is typically the second field (index 1) + level := parts[1] + + // The message is everything after the level + msgStart := strings.Index(logLine, level) + len(level) + msg := strings.TrimSpace(logLine[msgStart:]) + + capture(level, msg) +} + +// SetCaptureFunc sets the capture function for the underlying capture writer. +func (w *captureWriter) SetCaptureFunc(captureFunc LogCaptureFunc) { + w.mu.Lock() + defer w.mu.Unlock() + + w.captureFunc = captureFunc +} + +// NewLogger creates a new zerolog-based logger with console output. +// +// The logger is configured with: +// - ConsoleWriter output to os.Stderr +// - RFC3339 timestamp format +// - Info level as default +func NewLogger() (Logger, error) { + output := zerolog.ConsoleWriter{ + Out: os.Stderr, + TimeFormat: time.RFC3339, + NoColor: false, } - return nil + // Create the base logger with info level. + zerologLogger := zerolog.New(output). + With(). + Timestamp(). + Logger(). + Level(zerolog.InfoLevel) + + return &ZerologLogger{ + logger: zerologLogger, + output: output, + }, nil } -// Sugar returns a sugared logger for convenient key-value logging. -func (z *ZapLogger) Sugar() *zap.SugaredLogger { - // Return nil if the logger is uninitialized to prevent nil pointer dereference. - if z.Logger == nil { - return nil +// NewLoggerWithCapture creates a new zerolog-based logger that supports log capture. +// +// The returned logger uses a captureWriter that can send log messages to a callback +// for display in the TUI. This is useful for verbose mode where debug logs should +// appear within the TUI interface rather than being written directly to stderr. +func NewLoggerWithCapture() (Logger, *captureWriter, error) { + // Create a captureWriter that wraps stderr + captureWriter := &captureWriter{ + output: os.Stderr, + } + + // Create a ConsoleWriter that writes to the captureWriter + consoleWriter := zerolog.ConsoleWriter{ + Out: captureWriter, + TimeFormat: time.RFC3339, + NoColor: false, + } + + // Create the base logger with info level. + zerologLogger := zerolog.New(consoleWriter). + With(). + Timestamp(). + Logger(). + Level(zerolog.InfoLevel) + + logger := &ZerologLogger{ + logger: zerologLogger, + output: consoleWriter, + captureFunc: nil, + } + + // Set up the capture writer to call the logger's capture function + captureWriter.captureFunc = func(level, msg string) { + logger.mu.RLock() + capture := logger.captureFunc + logger.mu.RUnlock() + + if capture != nil { + capture(level, msg) + } } - // Delegate to zap.Logger’s Sugar method for sugared logging. - return z.Logger.Sugar() + return logger, captureWriter, nil +} + +// Debug returns a debug-level event for logging. +func (z *ZerologLogger) Debug() *zerolog.Event { + z.mu.RLock() + defer z.mu.RUnlock() + + return z.logger.Debug() +} + +// Info returns an info-level event for logging. +func (z *ZerologLogger) Info() *zerolog.Event { + z.mu.RLock() + defer z.mu.RUnlock() + + return z.logger.Info() +} + +// Warn returns a warn-level event for logging. +func (z *ZerologLogger) Warn() *zerolog.Event { + z.mu.RLock() + defer z.mu.RUnlock() + + return z.logger.Warn() +} + +// Error returns an error-level event for logging. +func (z *ZerologLogger) Error() *zerolog.Event { + z.mu.RLock() + defer z.mu.RUnlock() + + return z.logger.Error() +} + +// Sync is a no-op for zerolog, kept for interface compatibility. +// zerolog writes directly to the output without buffering. +func (z *ZerologLogger) Sync() error { + z.mu.RLock() + defer z.mu.RUnlock() + + return nil +} + +// Level sets the minimum log level dynamically. +func (z *ZerologLogger) Level(level zerolog.Level) { + z.mu.Lock() + defer z.mu.Unlock() + + // Create a new logger with the specified level. + z.logger = z.logger.Level(level) +} + +// SetCaptureFunc sets a callback function to capture log messages for TUI display. +// When set, log messages are parsed and sent to this callback. +func (z *ZerologLogger) SetCaptureFunc(captureFunc LogCaptureFunc) { + z.mu.Lock() + defer z.mu.Unlock() + + z.captureFunc = captureFunc +} + +// ParseLevel parses a string log level into a zerolog.Level. +// +// Supported levels (case-insensitive): +// - "debug" -> DebugLevel +// - "info" -> InfoLevel +// - "warn" -> WarnLevel +// - "error" -> ErrorLevel +// +// Defaults to InfoLevel if the level is unrecognized. +func ParseLevel(level string) zerolog.Level { + switch strings.ToLower(level) { + case "debug": + return zerolog.DebugLevel + case "info": + return zerolog.InfoLevel + case "warn": + return zerolog.WarnLevel + case "error": + return zerolog.ErrorLevel + default: + return zerolog.InfoLevel + } } diff --git a/internal/logger/logger_test.go b/internal/logger/logger_test.go index 67b68c6..2a228c0 100644 --- a/internal/logger/logger_test.go +++ b/internal/logger/logger_test.go @@ -18,14 +18,17 @@ along with this program. If not, see . package logger import ( - "reflect" + "bytes" + "strings" "testing" - "go.uber.org/zap" + "github.com/rs/zerolog" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) -// TestNewZapLogger verifies the NewZapLogger function’s logger creation. -func TestNewZapLogger(t *testing.T) { +// TestNewLogger verifies the NewLogger function creates a valid logger. +func TestNewLogger(t *testing.T) { tests := []struct { name string wantErr bool @@ -37,91 +40,348 @@ func TestNewZapLogger(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := NewZapLogger() - if (err != nil) != tt.wantErr { - t.Errorf("NewZapLogger() error = %v, wantErr %v", err, tt.wantErr) + got, err := NewLogger() + if tt.wantErr { + require.Error(t, err) return } - if got == nil { - t.Errorf("NewZapLogger() returned nil logger") + require.NoError(t, err) + assert.NotNil(t, got) - return + // Verify the returned logger is a *ZerologLogger. + _, ok := got.(*ZerologLogger) + assert.True(t, ok, "expected *ZerologLogger, got %T", got) + }) + } +} + +// TestZerologLogger_LogLevelMethods verifies all log level methods work correctly. +func TestZerologLogger_LogLevelMethods(t *testing.T) { + tests := []struct { + name string + level zerolog.Level + logFunc func(l Logger) *zerolog.Event + wantLog bool + contains string + }{ + { + name: "debug level with debug enabled", + level: zerolog.DebugLevel, + logFunc: func(l Logger) *zerolog.Event { return l.Debug() }, + wantLog: true, + contains: "DBG", + }, + { + name: "info level", + level: zerolog.InfoLevel, + logFunc: func(l Logger) *zerolog.Event { return l.Info() }, + wantLog: true, + contains: "INF", + }, + { + name: "warn level", + level: zerolog.WarnLevel, + logFunc: func(l Logger) *zerolog.Event { return l.Warn() }, + wantLog: true, + contains: "WRN", + }, + { + name: "error level", + level: zerolog.ErrorLevel, + logFunc: func(l Logger) *zerolog.Event { return l.Error() }, + wantLog: true, + contains: "ERR", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Create a buffer to capture log output. + var buf bytes.Buffer + + output := zerolog.ConsoleWriter{ + Out: &buf, + TimeFormat: "2006-01-02", + NoColor: true, } - // Verify the returned logger is a valid *ZapLogger. - zapLogger, ok := got.(*ZapLogger) - if !ok || zapLogger.Logger == nil { - t.Errorf("NewZapLogger() = %v, expected non-nil *ZapLogger", got) + // Create logger with specific level. + zl := zerolog.New(output). + With(). + Timestamp(). + Logger(). + Level(tt.level) + + logger := &ZerologLogger{logger: zl, output: output} + + // Call the log function with a test message. + tt.logFunc(logger).Msg("test message") + + outputStr := buf.String() + + if tt.wantLog { + assert.Contains(t, outputStr, tt.contains) + assert.Contains(t, outputStr, "test message") } }) } } -// TestZapLogger_Sync verifies the Sync method’s behavior. -func TestZapLogger_Sync(t *testing.T) { +// TestZerologLogger_Sync verifies the Sync method's behavior. +func TestZerologLogger_Sync(t *testing.T) { tests := []struct { name string - z *ZapLogger + setup func() *ZerologLogger wantErr bool }{ { name: "sync with valid logger", - z: &ZapLogger{ - Logger: zap.NewNop(), // Use no-op logger to avoid real I/O + setup: func() *ZerologLogger { + zl := zerolog.New(nil).With().Logger() + + return &ZerologLogger{logger: zl} }, wantErr: false, }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + z := tt.setup() + err := z.Sync() + assert.NoError(t, err) + }) + } +} + +// TestZerologLogger_Level verifies the Level method changes log level dynamically. +func TestZerologLogger_Level(t *testing.T) { + tests := []struct { + name string + initialLevel zerolog.Level + newLevel zerolog.Level + logLevel zerolog.Level + shouldLog bool + }{ + { + name: "change from info to debug", + initialLevel: zerolog.InfoLevel, + newLevel: zerolog.DebugLevel, + logLevel: zerolog.DebugLevel, + shouldLog: true, + }, { - name: "sync with nil logger", - z: &ZapLogger{nil}, - wantErr: true, + name: "change from debug to error", + initialLevel: zerolog.DebugLevel, + newLevel: zerolog.ErrorLevel, + logLevel: zerolog.InfoLevel, + shouldLog: false, + }, + { + name: "no change in level", + initialLevel: zerolog.InfoLevel, + newLevel: zerolog.InfoLevel, + logLevel: zerolog.InfoLevel, + shouldLog: true, }, } + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - err := tt.z.Sync() - if (err != nil) != tt.wantErr { - t.Errorf("ZapLogger.Sync() error = %v, wantErr %v", err, tt.wantErr) + var buf bytes.Buffer + + output := zerolog.ConsoleWriter{ + Out: &buf, + TimeFormat: "2006-01-02", + NoColor: true, + } + + // Create logger with initial level. + zl := zerolog.New(output). + With(). + Timestamp(). + Logger(). + Level(tt.initialLevel) + + logger := &ZerologLogger{logger: zl, output: output} + + // Change the level. + logger.Level(tt.newLevel) + + // Try to log at the specified level. + switch tt.logLevel { + case zerolog.DebugLevel: + logger.Debug().Msg("test debug") + case zerolog.InfoLevel: + logger.Info().Msg("test info") + case zerolog.ErrorLevel: + logger.Error().Msg("test error") + } + + outputStr := buf.String() + + if tt.shouldLog { + assert.NotEmpty(t, outputStr) + } else { + assert.Empty(t, outputStr) } }) } } -// TestZapLogger_Sugar verifies the Sugar method’s sugared logger output. -func TestZapLogger_Sugar(t *testing.T) { +// TestParseLevel verifies the ParseLevel function's string parsing. +func TestParseLevel(t *testing.T) { tests := []struct { - name string - z *ZapLogger + name string + level string + want zerolog.Level }{ { - name: "sugar with valid logger", - z: func() *ZapLogger { - logger, _ := zap.NewProduction() - - return &ZapLogger{logger} - }(), + name: "debug lowercase", + level: "debug", + want: zerolog.DebugLevel, + }, + { + name: "info lowercase", + level: "info", + want: zerolog.InfoLevel, }, { - name: "sugar with nil logger", - z: &ZapLogger{nil}, + name: "warn lowercase", + level: "warn", + want: zerolog.WarnLevel, + }, + { + name: "error lowercase", + level: "error", + want: zerolog.ErrorLevel, + }, + { + name: "debug uppercase", + level: "DEBUG", + want: zerolog.DebugLevel, + }, + { + name: "info mixed case", + level: "Info", + want: zerolog.InfoLevel, + }, + { + name: "unknown level defaults to info", + level: "unknown", + want: zerolog.InfoLevel, + }, + { + name: "empty string defaults to info", + level: "", + want: zerolog.InfoLevel, }, } + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := tt.z.Sugar() - if tt.z.Logger == nil { - if got != nil { - t.Errorf("ZapLogger.Sugar() = %v, want nil for nil logger", got) - } - } else { - if got == nil { - t.Errorf("ZapLogger.Sugar() = nil, want non-nil *zap.SugaredLogger") - } else if reflect.TypeOf(got) != reflect.TypeOf(zap.NewNop().Sugar()) { - t.Errorf("ZapLogger.Sugar() = %v, want *zap.SugaredLogger", got) - } - } + got := ParseLevel(tt.level) + assert.Equal(t, tt.want, got) }) } } + +// BenchmarkZerologLogger_Debug benchmarks the Debug method. +func BenchmarkZerologLogger_Debug(b *testing.B) { + zl := zerolog.New(nil).With().Logger() + logger := &ZerologLogger{logger: zl} + + b.ResetTimer() + + for b.Loop() { + logger.Debug().Msg("benchmark message") + } +} + +// BenchmarkZerologLogger_Info benchmarks the Info method. +func BenchmarkZerologLogger_Info(b *testing.B) { + zl := zerolog.New(nil).With().Logger() + logger := &ZerologLogger{logger: zl} + + b.ResetTimer() + + for b.Loop() { + logger.Info().Msg("benchmark message") + } +} + +// BenchmarkParseLevel benchmarks the ParseLevel function. +func BenchmarkParseLevel(b *testing.B) { + levels := []string{"debug", "info", "warn", "error", "unknown"} + + b.ResetTimer() + + for b.Loop() { + for _, level := range levels { + ParseLevel(level) + } + } +} + +// TestZerologLogger_ConcurrentAccess verifies thread safety of logger methods. +func TestZerologLogger_ConcurrentAccess(t *testing.T) { + var buf bytes.Buffer + + output := zerolog.ConsoleWriter{ + Out: &buf, + TimeFormat: "2006-01-02", + NoColor: true, + } + + zl := zerolog.New(output).With().Timestamp().Logger().Level(zerolog.DebugLevel) + logger := &ZerologLogger{logger: zl, output: output} + + // Run concurrent logging operations. + done := make(chan bool, 4) + + go func() { + for range 100 { + logger.Debug().Msg("debug message") + } + + done <- true + }() + + go func() { + for range 100 { + logger.Info().Msg("info message") + } + + done <- true + }() + + go func() { + for range 100 { + logger.Warn().Msg("warn message") + } + + done <- true + }() + + go func() { + for range 100 { + logger.Error().Msg("error message") + } + + done <- true + }() + + // Wait for all goroutines to complete. + for range 4 { + <-done + } + + // Verify output contains messages from all levels. + outputStr := buf.String() + assert.Positive(t, strings.Count(outputStr, "debug message")) + assert.Positive(t, strings.Count(outputStr, "info message")) + assert.Positive(t, strings.Count(outputStr, "warn message")) + assert.Positive(t, strings.Count(outputStr, "error message")) +} diff --git a/internal/logger/mocks/Logger.go b/internal/logger/mocks/Logger.go index 015ac67..02a0955 100644 --- a/internal/logger/mocks/Logger.go +++ b/internal/logger/mocks/Logger.go @@ -5,8 +5,9 @@ package mocks import ( + "github.com/nicholas-fedor/go-remove/internal/logger" + "github.com/rs/zerolog" mock "github.com/stretchr/testify/mock" - "go.uber.org/zap" ) // NewMockLogger creates a new instance of MockLogger. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. @@ -36,52 +37,224 @@ func (_m *MockLogger) EXPECT() *MockLogger_Expecter { return &MockLogger_Expecter{mock: &_m.Mock} } -// Sugar provides a mock function for the type MockLogger -func (_mock *MockLogger) Sugar() *zap.SugaredLogger { +// Debug provides a mock function for the type MockLogger +func (_mock *MockLogger) Debug() *zerolog.Event { ret := _mock.Called() if len(ret) == 0 { - panic("no return value specified for Sugar") + panic("no return value specified for Debug") } - var r0 *zap.SugaredLogger - if returnFunc, ok := ret.Get(0).(func() *zap.SugaredLogger); ok { + var r0 *zerolog.Event + if returnFunc, ok := ret.Get(0).(func() *zerolog.Event); ok { r0 = returnFunc() } else { if ret.Get(0) != nil { - r0 = ret.Get(0).(*zap.SugaredLogger) + r0 = ret.Get(0).(*zerolog.Event) } } return r0 } -// MockLogger_Sugar_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Sugar' -type MockLogger_Sugar_Call struct { +// MockLogger_Debug_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Debug' +type MockLogger_Debug_Call struct { *mock.Call } -// Sugar is a helper method to define mock.On call -func (_e *MockLogger_Expecter) Sugar() *MockLogger_Sugar_Call { - return &MockLogger_Sugar_Call{Call: _e.mock.On("Sugar")} +// Debug is a helper method to define mock.On call +func (_e *MockLogger_Expecter) Debug() *MockLogger_Debug_Call { + return &MockLogger_Debug_Call{Call: _e.mock.On("Debug")} } -func (_c *MockLogger_Sugar_Call) Run(run func()) *MockLogger_Sugar_Call { +func (_c *MockLogger_Debug_Call) Run(run func()) *MockLogger_Debug_Call { _c.Call.Run(func(args mock.Arguments) { run() }) return _c } -func (_c *MockLogger_Sugar_Call) Return(sugaredLogger *zap.SugaredLogger) *MockLogger_Sugar_Call { - _c.Call.Return(sugaredLogger) +func (_c *MockLogger_Debug_Call) Return(event *zerolog.Event) *MockLogger_Debug_Call { + _c.Call.Return(event) return _c } -func (_c *MockLogger_Sugar_Call) RunAndReturn(run func() *zap.SugaredLogger) *MockLogger_Sugar_Call { +func (_c *MockLogger_Debug_Call) RunAndReturn(run func() *zerolog.Event) *MockLogger_Debug_Call { _c.Call.Return(run) return _c } +// Error provides a mock function for the type MockLogger +func (_mock *MockLogger) Error() *zerolog.Event { + ret := _mock.Called() + + if len(ret) == 0 { + panic("no return value specified for Error") + } + + var r0 *zerolog.Event + if returnFunc, ok := ret.Get(0).(func() *zerolog.Event); ok { + r0 = returnFunc() + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*zerolog.Event) + } + } + return r0 +} + +// MockLogger_Error_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Error' +type MockLogger_Error_Call struct { + *mock.Call +} + +// Error is a helper method to define mock.On call +func (_e *MockLogger_Expecter) Error() *MockLogger_Error_Call { + return &MockLogger_Error_Call{Call: _e.mock.On("Error")} +} + +func (_c *MockLogger_Error_Call) Run(run func()) *MockLogger_Error_Call { + _c.Call.Run(func(args mock.Arguments) { + run() + }) + return _c +} + +func (_c *MockLogger_Error_Call) Return(event *zerolog.Event) *MockLogger_Error_Call { + _c.Call.Return(event) + return _c +} + +func (_c *MockLogger_Error_Call) RunAndReturn(run func() *zerolog.Event) *MockLogger_Error_Call { + _c.Call.Return(run) + return _c +} + +// Info provides a mock function for the type MockLogger +func (_mock *MockLogger) Info() *zerolog.Event { + ret := _mock.Called() + + if len(ret) == 0 { + panic("no return value specified for Info") + } + + var r0 *zerolog.Event + if returnFunc, ok := ret.Get(0).(func() *zerolog.Event); ok { + r0 = returnFunc() + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*zerolog.Event) + } + } + return r0 +} + +// MockLogger_Info_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Info' +type MockLogger_Info_Call struct { + *mock.Call +} + +// Info is a helper method to define mock.On call +func (_e *MockLogger_Expecter) Info() *MockLogger_Info_Call { + return &MockLogger_Info_Call{Call: _e.mock.On("Info")} +} + +func (_c *MockLogger_Info_Call) Run(run func()) *MockLogger_Info_Call { + _c.Call.Run(func(args mock.Arguments) { + run() + }) + return _c +} + +func (_c *MockLogger_Info_Call) Return(event *zerolog.Event) *MockLogger_Info_Call { + _c.Call.Return(event) + return _c +} + +func (_c *MockLogger_Info_Call) RunAndReturn(run func() *zerolog.Event) *MockLogger_Info_Call { + _c.Call.Return(run) + return _c +} + +// Level provides a mock function for the type MockLogger +func (_mock *MockLogger) Level(level zerolog.Level) { + _mock.Called(level) + return +} + +// MockLogger_Level_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Level' +type MockLogger_Level_Call struct { + *mock.Call +} + +// Level is a helper method to define mock.On call +// - level zerolog.Level +func (_e *MockLogger_Expecter) Level(level interface{}) *MockLogger_Level_Call { + return &MockLogger_Level_Call{Call: _e.mock.On("Level", level)} +} + +func (_c *MockLogger_Level_Call) Run(run func(level zerolog.Level)) *MockLogger_Level_Call { + _c.Call.Run(func(args mock.Arguments) { + var arg0 zerolog.Level + if args[0] != nil { + arg0 = args[0].(zerolog.Level) + } + run( + arg0, + ) + }) + return _c +} + +func (_c *MockLogger_Level_Call) Return() *MockLogger_Level_Call { + _c.Call.Return() + return _c +} + +func (_c *MockLogger_Level_Call) RunAndReturn(run func(level zerolog.Level)) *MockLogger_Level_Call { + _c.Run(run) + return _c +} + +// SetCaptureFunc provides a mock function for the type MockLogger +func (_mock *MockLogger) SetCaptureFunc(captureFunc logger.LogCaptureFunc) { + _mock.Called(captureFunc) + return +} + +// MockLogger_SetCaptureFunc_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'SetCaptureFunc' +type MockLogger_SetCaptureFunc_Call struct { + *mock.Call +} + +// SetCaptureFunc is a helper method to define mock.On call +// - captureFunc logger.LogCaptureFunc +func (_e *MockLogger_Expecter) SetCaptureFunc(captureFunc interface{}) *MockLogger_SetCaptureFunc_Call { + return &MockLogger_SetCaptureFunc_Call{Call: _e.mock.On("SetCaptureFunc", captureFunc)} +} + +func (_c *MockLogger_SetCaptureFunc_Call) Run(run func(captureFunc logger.LogCaptureFunc)) *MockLogger_SetCaptureFunc_Call { + _c.Call.Run(func(args mock.Arguments) { + var arg0 logger.LogCaptureFunc + if args[0] != nil { + arg0 = args[0].(logger.LogCaptureFunc) + } + run( + arg0, + ) + }) + return _c +} + +func (_c *MockLogger_SetCaptureFunc_Call) Return() *MockLogger_SetCaptureFunc_Call { + _c.Call.Return() + return _c +} + +func (_c *MockLogger_SetCaptureFunc_Call) RunAndReturn(run func(captureFunc logger.LogCaptureFunc)) *MockLogger_SetCaptureFunc_Call { + _c.Run(run) + return _c +} + // Sync provides a mock function for the type MockLogger func (_mock *MockLogger) Sync() error { ret := _mock.Called() @@ -125,3 +298,49 @@ func (_c *MockLogger_Sync_Call) RunAndReturn(run func() error) *MockLogger_Sync_ _c.Call.Return(run) return _c } + +// Warn provides a mock function for the type MockLogger +func (_mock *MockLogger) Warn() *zerolog.Event { + ret := _mock.Called() + + if len(ret) == 0 { + panic("no return value specified for Warn") + } + + var r0 *zerolog.Event + if returnFunc, ok := ret.Get(0).(func() *zerolog.Event); ok { + r0 = returnFunc() + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*zerolog.Event) + } + } + return r0 +} + +// MockLogger_Warn_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Warn' +type MockLogger_Warn_Call struct { + *mock.Call +} + +// Warn is a helper method to define mock.On call +func (_e *MockLogger_Expecter) Warn() *MockLogger_Warn_Call { + return &MockLogger_Warn_Call{Call: _e.mock.On("Warn")} +} + +func (_c *MockLogger_Warn_Call) Run(run func()) *MockLogger_Warn_Call { + _c.Call.Run(func(args mock.Arguments) { + run() + }) + return _c +} + +func (_c *MockLogger_Warn_Call) Return(event *zerolog.Event) *MockLogger_Warn_Call { + _c.Call.Return(event) + return _c +} + +func (_c *MockLogger_Warn_Call) RunAndReturn(run func() *zerolog.Event) *MockLogger_Warn_Call { + _c.Call.Return(run) + return _c +} From af33253c660ab433dc68a7a822f15d75ccc93d24 Mon Sep 17 00:00:00 2001 From: Nick Fedor Date: Mon, 2 Mar 2026 00:50:44 -0700 Subject: [PATCH 09/20] refactor(zerolog): enable zerologlint and fix dispatch violations - Enable zerologlint linter in golangci-lint configuration - Fix mock loggers to dispatch events with Msg() calls - Remove unused ErrInvalidLoggerType and type assertion - Add nolint comments for factory method returns - Fix revive linter empty slice syntax --- .golangci.yaml | 8 ++++---- internal/cli/cli.go | 4 ---- internal/cli/cli_test.go | 38 ++++++++++++++++++++++++-------------- internal/cli/tui_test.go | 27 +++++++++++++++++++++++---- internal/fs/fs_test.go | 2 ++ internal/logger/logger.go | 4 ++++ 6 files changed, 57 insertions(+), 26 deletions(-) diff --git a/.golangci.yaml b/.golangci.yaml index abcbe34..c561a90 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -102,6 +102,7 @@ linters: - varnamelen # Checks that the length of a variable's name matches its scope. - wastedassign # Finds wasted assignment statements. - wrapcheck # Checks that errors returned from external packages are wrapped. + - zerologlint # Detects the wrong usage of `zerolog` that a user forgets to dispatch with `Send` or `Msg`. disable: - cyclop # Checks function and package cyclomatic complexity. - depguard # Checks if package imports are in a list of acceptable packages. @@ -125,7 +126,6 @@ linters: - tagliatelle # Checks the struct tags. - testableexamples # Linter checks if examples are testable (have an expected output). [fast] - testpackage # Linter that makes you use a separate _test package. - - zerologlint # Detects the wrong usage of `zerolog` that a user forgets to dispatch with `Send` or `Msg`. ###################################################################################################### # Linter Settings Configuration @@ -140,10 +140,10 @@ linters: - name: var-naming severity: warning disabled: false - exclude: [""] + exclude: [] arguments: - - [""] # AllowList - - [""] # DenyList + - [] # AllowList + - [] # DenyList - - skip-initialism-name-checks: false upper-case-const: true skip-package-name-checks: true diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 57051e1..3805b35 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -19,7 +19,6 @@ along with this program. If not, see . package cli import ( - "errors" "fmt" "os" @@ -27,9 +26,6 @@ import ( "github.com/nicholas-fedor/go-remove/internal/logger" ) -// ErrInvalidLoggerType indicates that the logger is not of the expected *ZerologLogger type. -var ErrInvalidLoggerType = errors.New("logger is not a *ZerologLogger") - // Config holds command-line configuration options. type Config struct { Binary string // Binary name to remove; empty for TUI mode diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index 5a9d562..8cf5924 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -58,10 +58,29 @@ type mockLogger struct { nopLogger zerolog.Logger } -func (m *mockLogger) Debug() *zerolog.Event { return m.nopLogger.Debug() } -func (m *mockLogger) Info() *zerolog.Event { return m.nopLogger.Info() } -func (m *mockLogger) Warn() *zerolog.Event { return m.nopLogger.Warn() } -func (m *mockLogger) Error() *zerolog.Event { return m.nopLogger.Error() } +func (m *mockLogger) Debug() *zerolog.Event { + m.nopLogger.Debug().Msg("") + + return nil +} + +func (m *mockLogger) Info() *zerolog.Event { + m.nopLogger.Info().Msg("") + + return nil +} + +func (m *mockLogger) Warn() *zerolog.Event { + m.nopLogger.Warn().Msg("") + + return nil +} + +func (m *mockLogger) Error() *zerolog.Event { + m.nopLogger.Error().Msg("") + + return nil +} func (m *mockLogger) Level(_ zerolog.Level) {} func (m *mockLogger) Sync() error { m.syncCalled = true @@ -198,17 +217,8 @@ func TestRun(t *testing.T) { // Set log level based on config if verbose mode is enabled. if config.Verbose { - zl, ok := log.(*logger.ZerologLogger) - if !ok { - return fmt.Errorf( - "failed to set log level: %w with type %T", - ErrInvalidLoggerType, - log, - ) - } - level := logger.ParseLevel(config.LogLevel) - zl.Level(level) + log.Level(level) } binDir, err := deps.FS.DetermineBinDir(config.Goroot) diff --git a/internal/cli/tui_test.go b/internal/cli/tui_test.go index d304f5e..ca448f2 100644 --- a/internal/cli/tui_test.go +++ b/internal/cli/tui_test.go @@ -39,10 +39,29 @@ type tuiMockLogger struct { nopLogger zerolog.Logger } -func (m *tuiMockLogger) Debug() *zerolog.Event { return m.nopLogger.Debug() } -func (m *tuiMockLogger) Info() *zerolog.Event { return m.nopLogger.Info() } -func (m *tuiMockLogger) Warn() *zerolog.Event { return m.nopLogger.Warn() } -func (m *tuiMockLogger) Error() *zerolog.Event { return m.nopLogger.Error() } +func (m *tuiMockLogger) Debug() *zerolog.Event { + m.nopLogger.Debug().Msg("") + + return nil +} + +func (m *tuiMockLogger) Info() *zerolog.Event { + m.nopLogger.Info().Msg("") + + return nil +} + +func (m *tuiMockLogger) Warn() *zerolog.Event { + m.nopLogger.Warn().Msg("") + + return nil +} + +func (m *tuiMockLogger) Error() *zerolog.Event { + m.nopLogger.Error().Msg("") + + return nil +} func (m *tuiMockLogger) Sync() error { return nil } func (m *tuiMockLogger) Level(_ zerolog.Level) {} func (m *tuiMockLogger) SetCaptureFunc(_ logger.LogCaptureFunc) {} diff --git a/internal/fs/fs_test.go b/internal/fs/fs_test.go index 0c3e48e..399ff52 100644 --- a/internal/fs/fs_test.go +++ b/internal/fs/fs_test.go @@ -267,6 +267,7 @@ func TestRealFS_RemoveBinary(t *testing.T) { dummyEvent := zl.Debug() log.On("Debug").Return(dummyEvent) log.On("Info").Return(dummyEvent) + dummyEvent.Msg("") return log }, @@ -313,6 +314,7 @@ func TestRealFS_RemoveBinary_VerboseLogging(t *testing.T) { // Set up expectations for verbose logging calls. log.On("Debug").Return(dummyEvent) log.On("Info").Return(dummyEvent) + dummyEvent.Msg("") tmpDir := t.TempDir() tmpFile := filepath.Join(tmpDir, "testbin") diff --git a/internal/logger/logger.go b/internal/logger/logger.go index 4859508..b65b0dc 100644 --- a/internal/logger/logger.go +++ b/internal/logger/logger.go @@ -220,6 +220,7 @@ func (z *ZerologLogger) Debug() *zerolog.Event { z.mu.RLock() defer z.mu.RUnlock() + //nolint:zerologlint // Factory method returns event for chaining by design return z.logger.Debug() } @@ -228,6 +229,7 @@ func (z *ZerologLogger) Info() *zerolog.Event { z.mu.RLock() defer z.mu.RUnlock() + //nolint:zerologlint // Factory method returns event for chaining by design return z.logger.Info() } @@ -236,6 +238,7 @@ func (z *ZerologLogger) Warn() *zerolog.Event { z.mu.RLock() defer z.mu.RUnlock() + //nolint:zerologlint // Factory method returns event for chaining by design return z.logger.Warn() } @@ -244,6 +247,7 @@ func (z *ZerologLogger) Error() *zerolog.Event { z.mu.RLock() defer z.mu.RUnlock() + //nolint:zerologlint // Factory method returns event for chaining by design return z.logger.Error() } From 40ae8823f81a85cbd7db2993c2fbd08b04a5446b Mon Sep 17 00:00:00 2001 From: Nick Fedor Date: Mon, 2 Mar 2026 00:51:59 -0700 Subject: [PATCH 10/20] fix(tui): refresh grid layout on log events - Update grid layout when toggling log panel visibility with 'L' key - Update grid layout when receiving new log messages --- internal/cli/tui.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/cli/tui.go b/internal/cli/tui.go index 0faa853..d910886 100644 --- a/internal/cli/tui.go +++ b/internal/cli/tui.go @@ -264,6 +264,7 @@ func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { case "L": // Toggle log panel visibility. m.showLogs = !m.showLogs + m.updateGrid() case "enter": // Remove the selected binary and update the TUI state. @@ -310,6 +311,7 @@ func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { case LogMsg: // Add log message to the circular buffer. m.addLogEntry(msg) + m.updateGrid() // Continue polling for more log messages. // This ensures all pending logs are captured. From ab5e50c732ce09b0143d24d171315b7ced03eda6 Mon Sep 17 00:00:00 2001 From: Nick Fedor Date: Mon, 2 Mar 2026 00:55:23 -0700 Subject: [PATCH 11/20] test(logger): fix race condition in concurrent access test - Add syncBuffer type with mutex-protected Write and String methods - Update TestZerologLogger_ConcurrentAccess to use syncBuffer instead of bytes.Buffer --- internal/logger/logger_test.go | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/internal/logger/logger_test.go b/internal/logger/logger_test.go index 2a228c0..495e473 100644 --- a/internal/logger/logger_test.go +++ b/internal/logger/logger_test.go @@ -20,6 +20,7 @@ package logger import ( "bytes" "strings" + "sync" "testing" "github.com/rs/zerolog" @@ -27,6 +28,29 @@ import ( "github.com/stretchr/testify/require" ) +// syncBuffer is a thread-safe wrapper around bytes.Buffer for concurrent writes. +type syncBuffer struct { + buf bytes.Buffer + mu sync.Mutex +} + +// Write writes p to the buffer in a thread-safe manner. +// It implements the io.Writer interface. +func (sb *syncBuffer) Write(p []byte) (int, error) { + sb.mu.Lock() + defer sb.mu.Unlock() + + return sb.buf.Write(p) +} + +// String returns the buffer's contents in a thread-safe manner. +func (sb *syncBuffer) String() string { + sb.mu.Lock() + defer sb.mu.Unlock() + + return sb.buf.String() +} + // TestNewLogger verifies the NewLogger function creates a valid logger. func TestNewLogger(t *testing.T) { tests := []struct { @@ -327,10 +351,10 @@ func BenchmarkParseLevel(b *testing.B) { // TestZerologLogger_ConcurrentAccess verifies thread safety of logger methods. func TestZerologLogger_ConcurrentAccess(t *testing.T) { - var buf bytes.Buffer + buf := &syncBuffer{} output := zerolog.ConsoleWriter{ - Out: &buf, + Out: buf, TimeFormat: "2006-01-02", NoColor: true, } From b3f12133db4103f0a1ef83a3ea35588349e7ded0 Mon Sep 17 00:00:00 2001 From: Nick Fedor Date: Mon, 2 Mar 2026 01:12:17 -0700 Subject: [PATCH 12/20] fix(logger): fix race condition in log capture - Add captureEnabled flag for thread-safe state tracking in Write - Add captureWriter field to ZerologLogger for lifecycle management - Refactor SetCaptureFunc to use SetupCaptureBridge for safety --- internal/logger/logger.go | 80 ++++++++++++++++++++++++--------------- 1 file changed, 50 insertions(+), 30 deletions(-) diff --git a/internal/logger/logger.go b/internal/logger/logger.go index b65b0dc..fd9ba3e 100644 --- a/internal/logger/logger.go +++ b/internal/logger/logger.go @@ -59,30 +59,32 @@ type Logger interface { // ZerologLogger wraps zerolog.Logger to implement the Logger interface. type ZerologLogger struct { - logger zerolog.Logger - mu sync.RWMutex - output io.Writer - captureFunc LogCaptureFunc + logger zerolog.Logger + mu sync.RWMutex + output io.Writer + captureFunc LogCaptureFunc + captureWriter *captureWriter } // captureWriter wraps an io.Writer and captures written data for TUI display. type captureWriter struct { - mu sync.RWMutex - output io.Writer - captureFunc LogCaptureFunc + mu sync.RWMutex + output io.Writer + captureFunc LogCaptureFunc + captureEnabled bool } // Write implements io.Writer, writing to the underlying output and capturing the data. -// When a capture function is set, output is discarded to prevent duplicate logs in TUI mode. +// When capture is enabled, output is discarded to prevent duplicate logs in TUI mode. func (w *captureWriter) Write(data []byte) (int, error) { w.mu.RLock() output := w.output - capture := w.captureFunc + enabled := w.captureEnabled w.mu.RUnlock() - // If a capture function is set, discard the output to prevent duplicate logs. + // If capture is enabled, discard the output to prevent duplicate logs. // The log message will only be sent through the capture mechanism to the TUI log panel. - if capture != nil { + if enabled { w.captureLogMessage(string(data)) // Write to io.Discard to satisfy the writer interface without producing output. @@ -94,7 +96,7 @@ func (w *captureWriter) Write(data []byte) (int, error) { return bytesWritten, nil } - // No capture function set, write to the underlying output normally. + // Capture is not enabled, write to the underlying output normally. bytesWritten, err := output.Write(data) if err != nil { return bytesWritten, fmt.Errorf("failed to write to output: %w", err) @@ -137,11 +139,24 @@ func (w *captureWriter) captureLogMessage(logLine string) { } // SetCaptureFunc sets the capture function for the underlying capture writer. +// When a non-nil captureFunc is provided, capture is enabled and log messages +// will be sent to the callback instead of the normal output. func (w *captureWriter) SetCaptureFunc(captureFunc LogCaptureFunc) { w.mu.Lock() defer w.mu.Unlock() w.captureFunc = captureFunc + w.captureEnabled = captureFunc != nil +} + +// SetupCaptureBridge configures the capture writer with a bridge function that +// calls the provided callback. This is used internally to connect the capture +// writer to the logger's capture function. +func (w *captureWriter) SetupCaptureBridge(bridge func(level, msg string)) { + w.mu.Lock() + defer w.mu.Unlock() + + w.captureFunc = bridge } // NewLogger creates a new zerolog-based logger with console output. @@ -176,12 +191,13 @@ func NewLogger() (Logger, error) { // for display in the TUI. This is useful for verbose mode where debug logs should // appear within the TUI interface rather than being written directly to stderr. func NewLoggerWithCapture() (Logger, *captureWriter, error) { - // Create a captureWriter that wraps stderr + // Create a captureWriter that wraps stderr. + // captureFunc and captureEnabled are left as zero values (nil and false). captureWriter := &captureWriter{ output: os.Stderr, } - // Create a ConsoleWriter that writes to the captureWriter + // Create a ConsoleWriter that writes to the captureWriter. consoleWriter := zerolog.ConsoleWriter{ Out: captureWriter, TimeFormat: time.RFC3339, @@ -196,20 +212,10 @@ func NewLoggerWithCapture() (Logger, *captureWriter, error) { Level(zerolog.InfoLevel) logger := &ZerologLogger{ - logger: zerologLogger, - output: consoleWriter, - captureFunc: nil, - } - - // Set up the capture writer to call the logger's capture function - captureWriter.captureFunc = func(level, msg string) { - logger.mu.RLock() - capture := logger.captureFunc - logger.mu.RUnlock() - - if capture != nil { - capture(level, msg) - } + logger: zerologLogger, + output: consoleWriter, + captureFunc: nil, + captureWriter: captureWriter, } return logger, captureWriter, nil @@ -273,9 +279,23 @@ func (z *ZerologLogger) Level(level zerolog.Level) { // When set, log messages are parsed and sent to this callback. func (z *ZerologLogger) SetCaptureFunc(captureFunc LogCaptureFunc) { z.mu.Lock() - defer z.mu.Unlock() - z.captureFunc = captureFunc + z.mu.Unlock() + + // Set up the bridge function on the capture writer when a callback is provided. + // The bridge function reads the current captureFunc from the logger and calls it. + if z.captureWriter != nil && captureFunc != nil { + z.captureWriter.SetupCaptureBridge(func(level, msg string) { + z.mu.RLock() + capture := z.captureFunc + z.mu.RUnlock() + + if capture != nil { + capture(level, msg) + } + }) + z.captureWriter.SetCaptureFunc(captureFunc) + } } // ParseLevel parses a string log level into a zerolog.Level. From fb66b36473a27aa172dc188e6f4944132217f31a Mon Sep 17 00:00:00 2001 From: Nick Fedor Date: Mon, 2 Mar 2026 01:28:26 -0700 Subject: [PATCH 13/20] refactor(logger): simplify io.Discard error handling - Remove unnecessary error checking when writing to io.Discard - Return len(data) directly instead of capturing return values - Add explanatory comment about io.Discard.Write behavior --- internal/logger/logger.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/internal/logger/logger.go b/internal/logger/logger.go index fd9ba3e..f655b8c 100644 --- a/internal/logger/logger.go +++ b/internal/logger/logger.go @@ -88,12 +88,10 @@ func (w *captureWriter) Write(data []byte) (int, error) { w.captureLogMessage(string(data)) // Write to io.Discard to satisfy the writer interface without producing output. - bytesWritten, err := io.Discard.Write(data) - if err != nil { - return bytesWritten, fmt.Errorf("failed to write to discard: %w", err) - } + // io.Discard.Write always returns (len(p), nil), so we can safely ignore the error. + _, _ = io.Discard.Write(data) - return bytesWritten, nil + return len(data), nil } // Capture is not enabled, write to the underlying output normally. From d8682fb234f9f6dea0b3390c2ef1db84c04b3c8c Mon Sep 17 00:00:00 2001 From: Nick Fedor Date: Mon, 2 Mar 2026 01:31:50 -0700 Subject: [PATCH 14/20] refactor(logger): scan for log levels instead of fixed position - Replace fixed index parsing with token scanning for level detection - Add support for variable log formats with additional prefix fields - Default to LOG level when no known zerolog level is detected --- internal/logger/logger.go | 55 ++++++++++++++++++++++++++++++++------- 1 file changed, 45 insertions(+), 10 deletions(-) diff --git a/internal/logger/logger.go b/internal/logger/logger.go index f655b8c..a3014ab 100644 --- a/internal/logger/logger.go +++ b/internal/logger/logger.go @@ -103,8 +103,14 @@ func (w *captureWriter) Write(data []byte) (int, error) { return bytesWritten, nil } -// captureLogMessage parses and captures the log message. -// Expected format from ConsoleWriter: " ". +// captureLogMessage parses and captures the log message from zerolog ConsoleWriter output. +// +// Expected format from ConsoleWriter: " " +// where LEVEL is one of: DBG, INF, WRN, ERR, FTL (zerolog's default level abbreviations). +// +// This function scans fields to find a known level token rather than assuming a fixed +// position, making it tolerant to format changes (e.g., additional prefix fields). +// If no known level token is found, it defaults to level "LOG" with the full message. func (w *captureWriter) captureLogMessage(logLine string) { w.mu.RLock() capture := w.captureFunc @@ -114,24 +120,53 @@ func (w *captureWriter) captureLogMessage(logLine string) { return } - // Parse the log line to extract level and message - // Format: "2006-01-02T15:04:05Z07:00 DBG message here" + // Known zerolog level abbreviations used by ConsoleWriter. + // These are the default short level names output by zerolog. + knownLevels := map[string]bool{ + "DBG": true, + "INF": true, + "WRN": true, + "ERR": true, + "FTL": true, + } + + // Parse the log line to extract level and message. + // Expected format: "2006-01-02T15:04:05Z07:00 DBG message here" parts := strings.Fields(logLine) const minLogParts = 3 // timestamp, level, message if len(parts) < minLogParts { - // Not enough parts, use the whole line + // Not enough parts, use the whole line as a generic log entry. capture("LOG", strings.TrimSpace(logLine)) return } - // The level is typically the second field (index 1) - level := parts[1] + // Scan for a known level token instead of assuming fixed position. + // This makes parsing more robust against format changes. + level := "LOG" // default level if no known token found + levelIndex := -1 + + for i, part := range parts { + if knownLevels[part] { + level = part + levelIndex = i - // The message is everything after the level - msgStart := strings.Index(logLine, level) + len(level) - msg := strings.TrimSpace(logLine[msgStart:]) + break + } + } + + var msg string + + if levelIndex >= 0 { + // Level found: message is everything after the level field. + // Reconstruct by finding the level in the original line and taking everything after it. + msgStart := strings.Index(logLine, level) + len(level) + msg = strings.TrimSpace(logLine[msgStart:]) + } else { + // No known level found: use the whole line as the message. + msg = strings.TrimSpace(logLine) + } capture(level, msg) } From f90d3104bd1a752d064b1970adff3c3061c00f1d Mon Sep 17 00:00:00 2001 From: Nick Fedor Date: Mon, 2 Mar 2026 01:40:43 -0700 Subject: [PATCH 15/20] refactor(cli): remove unused program field from TUI model - Remove program field storing tea.Program reference from model struct - Remove program assignment initialization in RunTUI function --- internal/cli/tui.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/internal/cli/tui.go b/internal/cli/tui.go index d910886..ff36c6b 100644 --- a/internal/cli/tui.go +++ b/internal/cli/tui.go @@ -87,7 +87,6 @@ type model struct { sortAscending bool // True for ascending sort, false for descending logs []string // Captured log messages (circular buffer) showLogs bool // Toggle log panel visibility - program *tea.Program // Reference to the Bubble Tea program for sending messages logChan chan LogMsg // Channel for receiving log messages from the logger } @@ -156,9 +155,6 @@ func RunTUI( return nil } - // Store program reference for log message sending. - m.program = program - // Run the program and capture any runtime errors. _, err = program.Run() if err != nil { From bf5f7405955492732c45b182bff2cc28d716b608 Mon Sep 17 00:00:00 2001 From: Nick Fedor Date: Mon, 2 Mar 2026 03:02:55 -0700 Subject: [PATCH 16/20] test(cli): refactor to use generated mocks - Replace custom mockLogger and cliMockRunner with generated mockery mocks - Add newMockLoggerWithDefaults helper to reduce test setup boilerplate - Refactor test table to use setup functions for dependency initialization - Update assertions to verify all mock expectations are satisfied --- .golangci.yaml | 1 + internal/cli/cli_test.go | 271 +++++++++++++++++++++------------------ 2 files changed, 144 insertions(+), 128 deletions(-) diff --git a/.golangci.yaml b/.golangci.yaml index c561a90..dbaa5ec 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -232,6 +232,7 @@ linters: - promlinter - wrapcheck - varnamelen + - zerologlint # Run some linter only for test files by excluding its issues for everything else. # - path-except: _test\.go diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index 8cf5924..3cc2faa 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -21,6 +21,7 @@ import ( "bytes" "errors" "fmt" + "io" "os" "testing" @@ -29,169 +30,157 @@ import ( tea "charm.land/bubbletea/v2" + mockRunner "github.com/nicholas-fedor/go-remove/internal/cli/mocks" mockFS "github.com/nicholas-fedor/go-remove/internal/fs/mocks" "github.com/nicholas-fedor/go-remove/internal/logger" + mockLogger "github.com/nicholas-fedor/go-remove/internal/logger/mocks" ) -// cliMockRunner mocks the ProgramRunner interface for CLI tests. -type cliMockRunner struct { - runProgram func(m tea.Model, opts ...tea.ProgramOption) (*tea.Program, error) -} - -// RunProgram executes the mock runner's program function. -func (m cliMockRunner) RunProgram( - model tea.Model, - opts ...tea.ProgramOption, -) (*tea.Program, error) { - return m.runProgram(model, opts...) -} - // mockNoOpRunner provides a no-op runner for TUI tests. func mockNoOpRunner(tea.Model, ...tea.ProgramOption) (*tea.Program, error) { - return nil, nil //nolint:nilnil // Mock no-op runner + return nil, nil //nolint:nilnil // Mock no-op runner returns nil values for test simplicity } -// mockLogger implements a simple mock logger for testing. -type mockLogger struct { - syncCalled bool - syncError error - nopLogger zerolog.Logger -} +// newMockLoggerWithDefaults creates a MockLogger with default expectations for all methods. +// This helper reduces boilerplate when setting up logger mocks for tests that don't need +// to verify specific logger interactions. +func newMockLoggerWithDefaults(t *testing.T) *mockLogger.MockLogger { + t.Helper() -func (m *mockLogger) Debug() *zerolog.Event { - m.nopLogger.Debug().Msg("") + m := mockLogger.NewMockLogger(t) - return nil -} + // Create a nop logger and get event pointers to use as return values. + nopLog := zerolog.New(io.Discard) -func (m *mockLogger) Info() *zerolog.Event { - m.nopLogger.Info().Msg("") + // Use Maybe() for optional methods that may not be called during tests. + m.On("Debug").Return(nopLog.Debug()).Maybe() + m.On("Info").Return(nopLog.Info()).Maybe() + m.On("Warn").Return(nopLog.Warn()).Maybe() + m.On("Error").Return(nopLog.Error()).Maybe() + m.On("Sync").Return(nil) + m.On("Level", mock.Anything).Return().Maybe() + m.On("SetCaptureFunc", mock.Anything).Return().Maybe() - return nil + return m } -func (m *mockLogger) Warn() *zerolog.Event { - m.nopLogger.Warn().Msg("") - - return nil -} - -func (m *mockLogger) Error() *zerolog.Event { - m.nopLogger.Error().Msg("") - - return nil -} -func (m *mockLogger) Level(_ zerolog.Level) {} -func (m *mockLogger) Sync() error { - m.syncCalled = true - - return m.syncError -} -func (m *mockLogger) SetCaptureFunc(_ logger.LogCaptureFunc) {} - // TestRun verifies the Run function's behavior under various conditions. +// +//nolint:thelper // Subtest functions in table-driven tests require *testing.T parameter for mock constructors func TestRun(t *testing.T) { tests := []struct { - name string - config Config - deps Dependencies - runner ProgramRunner - wantErr bool - wantOutput string // Expected stdout output for non-verbose success + name string + config Config + setupFS func(t *testing.T) *mockFS.MockFS + setupLog func(t *testing.T) *mockLogger.MockLogger + setupRunner func(t *testing.T) *mockRunner.MockProgramRunner + wantErr bool + wantOutput string // Expected stdout output for non-verbose success }{ { name: "direct removal success", config: Config{Binary: "vhs", Verbose: false, Goroot: false}, - deps: Dependencies{ - FS: func() *mockFS.MockFS { - m := mockFS.NewMockFS(t) - m.On("DetermineBinDir", false).Return("/bin", nil) - m.On("AdjustBinaryPath", "/bin", "vhs").Return("/bin/vhs") - m.On("RemoveBinary", "/bin/vhs", "vhs", false, mock.Anything).Return(nil) - - return m - }(), - Logger: &mockLogger{}, + setupFS: func(t *testing.T) *mockFS.MockFS { + m := mockFS.NewMockFS(t) + m.On("DetermineBinDir", false).Return("/bin", nil) + m.On("AdjustBinaryPath", "/bin", "vhs").Return("/bin/vhs") + m.On("RemoveBinary", "/bin/vhs", "vhs", false, mock.Anything).Return(nil) + + return m }, + setupLog: newMockLoggerWithDefaults, wantErr: false, wantOutput: "Successfully removed vhs\n", }, { name: "direct removal failure", config: Config{Binary: "vhs", Verbose: false, Goroot: false}, - deps: Dependencies{ - FS: func() *mockFS.MockFS { - m := mockFS.NewMockFS(t) - m.On("DetermineBinDir", false).Return("/bin", nil) - m.On("AdjustBinaryPath", "/bin", "vhs").Return("/bin/vhs") - m.On("RemoveBinary", "/bin/vhs", "vhs", false, mock.Anything). - Return(errors.New("remove failed")) - - return m - }(), - Logger: &mockLogger{}, + setupFS: func(t *testing.T) *mockFS.MockFS { + m := mockFS.NewMockFS(t) + m.On("DetermineBinDir", false).Return("/bin", nil) + m.On("AdjustBinaryPath", "/bin", "vhs").Return("/bin/vhs") + m.On("RemoveBinary", "/bin/vhs", "vhs", false, mock.Anything). + Return(errors.New("remove failed")) + + return m }, - wantErr: true, + setupLog: newMockLoggerWithDefaults, + wantErr: true, }, { name: "tui mode success", config: Config{Binary: "", Verbose: false, Goroot: false}, - deps: Dependencies{ - FS: func() *mockFS.MockFS { - m := mockFS.NewMockFS(t) - m.On("DetermineBinDir", false).Return("/bin", nil) - m.On("ListBinaries", "/bin").Return([]string{"vhs"}) - - return m - }(), - Logger: &mockLogger{}, + setupFS: func(t *testing.T) *mockFS.MockFS { + m := mockFS.NewMockFS(t) + m.On("DetermineBinDir", false).Return("/bin", nil) + m.On("ListBinaries", "/bin").Return([]string{"vhs"}) + + return m + }, + setupLog: newMockLoggerWithDefaults, + setupRunner: func(t *testing.T) *mockRunner.MockProgramRunner { + m := mockRunner.NewMockProgramRunner(t) + m.On("RunProgram", mock.Anything, mock.Anything).Return(nil, nil) + + return m }, - runner: &cliMockRunner{runProgram: mockNoOpRunner}, wantErr: false, }, { name: "tui mode no binaries", config: Config{Binary: "", Verbose: false, Goroot: false}, - deps: Dependencies{ - FS: func() *mockFS.MockFS { - m := mockFS.NewMockFS(t) - m.On("DetermineBinDir", false).Return("/bin", nil) - m.On("ListBinaries", "/bin").Return([]string{}) - - return m - }(), - Logger: &mockLogger{}, + setupFS: func(t *testing.T) *mockFS.MockFS { + m := mockFS.NewMockFS(t) + m.On("DetermineBinDir", false).Return("/bin", nil) + m.On("ListBinaries", "/bin").Return([]string{}) + + return m + }, + setupLog: newMockLoggerWithDefaults, + setupRunner: func(t *testing.T) *mockRunner.MockProgramRunner { + m := mockRunner.NewMockProgramRunner(t) + // RunProgram may not be called if RunTUI returns an error early + m.On("RunProgram", mock.Anything, mock.Anything).Return(nil, nil).Maybe() + + return m }, - runner: &cliMockRunner{runProgram: mockNoOpRunner}, wantErr: true, }, { name: "bin dir error", config: Config{Binary: "vhs", Verbose: false, Goroot: false}, - deps: Dependencies{ - FS: func() *mockFS.MockFS { - m := mockFS.NewMockFS(t) - m.On("DetermineBinDir", false).Return("", errors.New("bin dir failed")) - - return m - }(), - Logger: &mockLogger{}, + setupFS: func(t *testing.T) *mockFS.MockFS { + m := mockFS.NewMockFS(t) + m.On("DetermineBinDir", false).Return("", errors.New("bin dir failed")) + + return m }, - wantErr: true, + setupLog: newMockLoggerWithDefaults, + wantErr: true, }, { name: "logger sync error", config: Config{Binary: "vhs", Verbose: false, Goroot: false}, - deps: Dependencies{ - FS: func() *mockFS.MockFS { - m := mockFS.NewMockFS(t) - m.On("DetermineBinDir", false).Return("/bin", nil) - m.On("AdjustBinaryPath", "/bin", "vhs").Return("/bin/vhs") - m.On("RemoveBinary", "/bin/vhs", "vhs", false, mock.Anything).Return(nil) - - return m - }(), - Logger: &mockLogger{syncError: errors.New("sync failed")}, + setupFS: func(t *testing.T) *mockFS.MockFS { + m := mockFS.NewMockFS(t) + m.On("DetermineBinDir", false).Return("/bin", nil) + m.On("AdjustBinaryPath", "/bin", "vhs").Return("/bin/vhs") + m.On("RemoveBinary", "/bin/vhs", "vhs", false, mock.Anything).Return(nil) + + return m + }, + setupLog: func(t *testing.T) *mockLogger.MockLogger { + m := mockLogger.NewMockLogger(t) + nopLog := zerolog.New(io.Discard) + m.On("Debug").Return(nopLog.Debug()).Maybe() + m.On("Info").Return(nopLog.Info()).Maybe() + m.On("Warn").Return(nopLog.Warn()).Maybe() + m.On("Error").Return(nopLog.Error()).Maybe() + m.On("Sync").Return(errors.New("sync failed")) + m.On("Level", mock.Anything).Return().Maybe() + m.On("SetCaptureFunc", mock.Anything).Return().Maybe() + + return m }, wantErr: false, // Sync errors are ignored on all platforms wantOutput: "Successfully removed vhs\n", @@ -211,6 +200,14 @@ func TestRun(t *testing.T) { w.Close() }() + // Set up dependencies using mock constructors. + mockFSInstance := tt.setupFS(t) + mockLog := tt.setupLog(t) + deps := Dependencies{ + FS: mockFSInstance, + Logger: mockLog, + } + // Define a local run function to test with a custom runner. run := func(deps Dependencies, config Config, runner ProgramRunner) error { log := deps.Logger @@ -250,14 +247,14 @@ func TestRun(t *testing.T) { return nil } - // Use DefaultRunner if no mock is provided. - runner := tt.runner - if runner == nil { - runner = DefaultRunner{} + // Use DefaultRunner if no mock is provided, otherwise use the mock runner. + var runner ProgramRunner = DefaultRunner{} + if tt.setupRunner != nil { + runner = tt.setupRunner(t) } // Execute the run function and capture any errors. - err := run(tt.deps, tt.config, runner) + err := run(deps, tt.config, runner) // Capture stdout output after execution. w.Close() @@ -277,23 +274,31 @@ func TestRun(t *testing.T) { } // Assert that all mock expectations were met. - tt.deps.FS.(*mockFS.MockFS).AssertExpectations(t) + mockFSInstance.AssertExpectations(t) + mockLog.AssertExpectations(t) + + // If using a mock runner, assert its expectations as well. + if mr, ok := runner.(*mockRunner.MockProgramRunner); ok { + mr.AssertExpectations(t) + } }) } } // TestRun_WithLoggerSync verifies that Sync is called appropriately. +// +//nolint:thelper // Subtest functions in table-driven tests require *testing.T parameter for mock constructors func TestRun_WithLoggerSync(t *testing.T) { tests := []struct { name string config Config - setupFS func() *mockFS.MockFS + setupFS func(t *testing.T) *mockFS.MockFS expectSyncCall bool }{ { name: "sync called on success", config: Config{Binary: "tool", Verbose: false, Goroot: false}, - setupFS: func() *mockFS.MockFS { + setupFS: func(t *testing.T) *mockFS.MockFS { m := mockFS.NewMockFS(t) m.On("DetermineBinDir", false).Return("/bin", nil) m.On("AdjustBinaryPath", "/bin", "tool").Return("/bin/tool") @@ -306,7 +311,7 @@ func TestRun_WithLoggerSync(t *testing.T) { { name: "sync called on error", config: Config{Binary: "tool", Verbose: false, Goroot: false}, - setupFS: func() *mockFS.MockFS { + setupFS: func(t *testing.T) *mockFS.MockFS { m := mockFS.NewMockFS(t) m.On("DetermineBinDir", false).Return("", errors.New("bin dir error")) @@ -318,9 +323,12 @@ func TestRun_WithLoggerSync(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - mockLog := &mockLogger{} + mockLog := mockLogger.NewMockLogger(t) + mockLog.On("Sync").Return(nil) + + mockFSInstance := tt.setupFS(t) deps := Dependencies{ - FS: tt.setupFS(), + FS: mockFSInstance, Logger: mockLog, } @@ -335,9 +343,8 @@ func TestRun_WithLoggerSync(t *testing.T) { _ = deps.FS.RemoveBinary(binaryPath, tt.config.Binary, tt.config.Verbose, deps.Logger) _ = deps.Logger.Sync() - if tt.expectSyncCall && !mockLog.syncCalled { - t.Errorf("Expected Sync() to be called") - } + // Assert that Sync was called on the mock logger. + mockLog.AssertCalled(t, "Sync") }) } } @@ -349,7 +356,11 @@ func TestRun_VerboseMode(t *testing.T) { m.On("AdjustBinaryPath", "/bin", "vhs").Return("/bin/vhs") m.On("RemoveBinary", "/bin/vhs", "vhs", true, mock.Anything).Return(nil) - mockLog := &mockLogger{} + mockLog := mockLogger.NewMockLogger(t) + // Level is optional since the test doesn't go through the full Run() path + mockLog.On("Level", mock.Anything).Return().Maybe() + mockLog.On("Sync").Return(nil) + deps := Dependencies{ FS: m, Logger: mockLog, @@ -381,4 +392,8 @@ func TestRun_VerboseMode(t *testing.T) { if buf.String() != "" { t.Errorf("Expected no stdout output in verbose mode, got: %q", buf.String()) } + + // Assert that all mock expectations were met. + m.AssertExpectations(t) + mockLog.AssertExpectations(t) } From cc52925410b33f9ee4fa476f49c9f1a2451af499 Mon Sep 17 00:00:00 2001 From: Nick Fedor Date: Mon, 2 Mar 2026 03:07:41 -0700 Subject: [PATCH 17/20] test(fs): simplify zerolog mock setup in tests - Remove intermediate dummyEvent variables - Return zerolog events directly from mock configurations - Eliminate redundant Msg("") calls on discarded events --- internal/fs/fs_test.go | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/internal/fs/fs_test.go b/internal/fs/fs_test.go index 399ff52..dd7bf00 100644 --- a/internal/fs/fs_test.go +++ b/internal/fs/fs_test.go @@ -264,10 +264,8 @@ func TestRealFS_RemoveBinary(t *testing.T) { // Create mock with expectations for verbose logging. log := mocks.NewMockLogger(t) zl := zerolog.New(nil).With().Logger() - dummyEvent := zl.Debug() - log.On("Debug").Return(dummyEvent) - log.On("Info").Return(dummyEvent) - dummyEvent.Msg("") + log.On("Debug").Return(zl.Debug()) + log.On("Info").Return(zl.Info()) return log }, @@ -307,14 +305,11 @@ func TestRealFS_RemoveBinary(t *testing.T) { func TestRealFS_RemoveBinary_VerboseLogging(t *testing.T) { log := mocks.NewMockLogger(t) - // Create a dummy zerolog logger and event for return values. zl := zerolog.New(nil).With().Logger() - dummyEvent := zl.Debug() // Set up expectations for verbose logging calls. - log.On("Debug").Return(dummyEvent) - log.On("Info").Return(dummyEvent) - dummyEvent.Msg("") + log.On("Debug").Return(zl.Debug()) + log.On("Info").Return(zl.Info()) tmpDir := t.TempDir() tmpFile := filepath.Join(tmpDir, "testbin") From 3772006e776ea1024630b898faa2dedabaf5d9d8 Mon Sep 17 00:00:00 2001 From: Nick Fedor Date: Mon, 2 Mar 2026 03:33:10 -0700 Subject: [PATCH 18/20] test(cli): refactor test helpers and improve mock integration - Extract captureStdout, makeDeps, and executeRun test helpers - Update FS test logger factory to accept *testing.T parameter - Fix logger SetCaptureFunc to properly disable capture with nil --- internal/cli/cli_test.go | 195 +++++++++++++++++++++----------------- internal/fs/fs_test.go | 16 ++-- internal/logger/logger.go | 5 +- 3 files changed, 119 insertions(+), 97 deletions(-) diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index 3cc2faa..36584a1 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -64,6 +64,96 @@ func newMockLoggerWithDefaults(t *testing.T) *mockLogger.MockLogger { return m } +// captureStdout redirects os.Stdout and returns a function that restores stdout +// and returns the captured output as a string. +func captureStdout(t *testing.T) func() string { + t.Helper() + + oldStdout := os.Stdout + r, w, _ := os.Pipe() + os.Stdout = w + + return func() string { + os.Stdout = oldStdout + + w.Close() + + var buf bytes.Buffer + buf.ReadFrom(r) + + return buf.String() + } +} + +// makeDeps creates Dependencies from the provided setup functions. +type makeDepsConfig struct { + setupFS func(t *testing.T) *mockFS.MockFS + setupLog func(t *testing.T) *mockLogger.MockLogger + setupRunner func(t *testing.T) *mockRunner.MockProgramRunner +} + +func makeDeps( + t *testing.T, + cfg makeDepsConfig, +) (Dependencies, *mockFS.MockFS, *mockLogger.MockLogger, ProgramRunner) { + t.Helper() + + mockFSInstance := cfg.setupFS(t) + mockLog := cfg.setupLog(t) + + deps := Dependencies{ + FS: mockFSInstance, + Logger: mockLog, + } + + var runner ProgramRunner = DefaultRunner{} + if cfg.setupRunner != nil { + runner = cfg.setupRunner(t) + } + + return deps, mockFSInstance, mockLog, runner +} + +// executeRun wraps the local run logic and returns the error. +// It mirrors the behavior of the Run function but accepts a custom runner for testing. +func executeRun(deps Dependencies, config Config, runner ProgramRunner) error { + log := deps.Logger + + // Set log level based on config if verbose mode is enabled. + if config.Verbose { + level := logger.ParseLevel(config.LogLevel) + log.Level(level) + } + + binDir, err := deps.FS.DetermineBinDir(config.Goroot) + if err != nil { + _ = log.Sync() // Flush logs; errors are ignored + + return err + } + + if config.Binary == "" { + err = RunTUI(binDir, config, log, deps.FS, runner) + } else { + binaryPath := deps.FS.AdjustBinaryPath(binDir, config.Binary) + + err = deps.FS.RemoveBinary(binaryPath, config.Binary, config.Verbose, log) + if err == nil && !config.Verbose { + fmt.Fprintf(os.Stdout, "Successfully removed %s\n", config.Binary) + } + } + + if err != nil { + _ = log.Sync() // Flush logs; errors are ignored + + return err + } + + _ = log.Sync() // Errors are ignored + + return nil +} + // TestRun verifies the Run function's behavior under various conditions. // //nolint:thelper // Subtest functions in table-driven tests require *testing.T parameter for mock constructors @@ -189,79 +279,21 @@ func TestRun(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - // Redirect stdout to capture output for non-verbose success cases. - oldStdout := os.Stdout - r, w, _ := os.Pipe() - os.Stdout = w - - defer func() { - os.Stdout = oldStdout - - w.Close() - }() - - // Set up dependencies using mock constructors. - mockFSInstance := tt.setupFS(t) - mockLog := tt.setupLog(t) - deps := Dependencies{ - FS: mockFSInstance, - Logger: mockLog, - } - - // Define a local run function to test with a custom runner. - run := func(deps Dependencies, config Config, runner ProgramRunner) error { - log := deps.Logger + // Capture stdout for output verification. + getOutput := captureStdout(t) - // Set log level based on config if verbose mode is enabled. - if config.Verbose { - level := logger.ParseLevel(config.LogLevel) - log.Level(level) - } - - binDir, err := deps.FS.DetermineBinDir(config.Goroot) - if err != nil { - _ = log.Sync() // Flush logs; errors are ignored - - return err - } - - if config.Binary == "" { - err = RunTUI(binDir, config, log, deps.FS, runner) - } else { - binaryPath := deps.FS.AdjustBinaryPath(binDir, config.Binary) - - err = deps.FS.RemoveBinary(binaryPath, config.Binary, config.Verbose, log) - if err == nil && !config.Verbose { - fmt.Fprintf(os.Stdout, "Successfully removed %s\n", config.Binary) - } - } - - if err != nil { - _ = log.Sync() // Flush logs; errors are ignored - - return err - } - - _ = log.Sync() // Errors are ignored - - return nil - } - - // Use DefaultRunner if no mock is provided, otherwise use the mock runner. - var runner ProgramRunner = DefaultRunner{} - if tt.setupRunner != nil { - runner = tt.setupRunner(t) - } + // Set up dependencies and runner. + deps, mockFSInstance, mockLog, runner := makeDeps(t, makeDepsConfig{ + setupFS: tt.setupFS, + setupLog: tt.setupLog, + setupRunner: tt.setupRunner, + }) // Execute the run function and capture any errors. - err := run(deps, tt.config, runner) + err := executeRun(deps, tt.config, runner) // Capture stdout output after execution. - w.Close() - - var buf bytes.Buffer - buf.ReadFrom(r) - gotOutput := buf.String() + gotOutput := getOutput() // Verify error behavior matches expectations. if (err != nil) != tt.wantErr { @@ -290,10 +322,9 @@ func TestRun(t *testing.T) { //nolint:thelper // Subtest functions in table-driven tests require *testing.T parameter for mock constructors func TestRun_WithLoggerSync(t *testing.T) { tests := []struct { - name string - config Config - setupFS func(t *testing.T) *mockFS.MockFS - expectSyncCall bool + name string + config Config + setupFS func(t *testing.T) *mockFS.MockFS }{ { name: "sync called on success", @@ -306,7 +337,6 @@ func TestRun_WithLoggerSync(t *testing.T) { return m }, - expectSyncCall: true, }, { name: "sync called on error", @@ -317,7 +347,6 @@ func TestRun_WithLoggerSync(t *testing.T) { return m }, - expectSyncCall: true, }, } @@ -367,30 +396,24 @@ func TestRun_VerboseMode(t *testing.T) { } config := Config{Binary: "vhs", Verbose: true, Goroot: false} - // Redirect stdout to capture output. - oldStdout := os.Stdout - r, w, _ := os.Pipe() - os.Stdout = w + // Capture stdout to capture output. + getOutput := captureStdout(t) binDir, _ := deps.FS.DetermineBinDir(config.Goroot) binaryPath := deps.FS.AdjustBinaryPath(binDir, config.Binary) err := deps.FS.RemoveBinary(binaryPath, config.Binary, config.Verbose, deps.Logger) _ = deps.Logger.Sync() - w.Close() - - os.Stdout = oldStdout - - var buf bytes.Buffer - buf.ReadFrom(r) + // Restore stdout and get captured output. + gotOutput := getOutput() if err != nil { t.Errorf("Unexpected error: %v", err) } // In verbose mode, no success message should be printed to stdout. - if buf.String() != "" { - t.Errorf("Expected no stdout output in verbose mode, got: %q", buf.String()) + if gotOutput != "" { + t.Errorf("Expected no stdout output in verbose mode, got: %q", gotOutput) } // Assert that all mock expectations were met. diff --git a/internal/fs/fs_test.go b/internal/fs/fs_test.go index dd7bf00..a667589 100644 --- a/internal/fs/fs_test.go +++ b/internal/fs/fs_test.go @@ -212,7 +212,7 @@ func TestRealFS_RemoveBinary(t *testing.T) { binaryPath string name string verbose bool - logger func() logger.Logger // Factory function to create logger + logger func(t *testing.T) logger.Logger // Factory function to create logger } tests := []struct { @@ -228,9 +228,7 @@ func TestRealFS_RemoveBinary(t *testing.T) { args: args{ name: "testbin", verbose: false, - logger: func() logger.Logger { - return nopLogger(t) - }, + logger: nopLogger, }, setup: func() string { tmpDir := t.TempDir() @@ -248,9 +246,7 @@ func TestRealFS_RemoveBinary(t *testing.T) { binaryPath: "/nonexistent/testbin", name: "testbin", verbose: false, - logger: func() logger.Logger { - return nopLogger(t) - }, + logger: nopLogger, }, wantErr: true, }, @@ -260,7 +256,9 @@ func TestRealFS_RemoveBinary(t *testing.T) { args: args{ name: "testbin", verbose: true, - logger: func() logger.Logger { + logger: func(t *testing.T) logger.Logger { + t.Helper() + // Create mock with expectations for verbose logging. log := mocks.NewMockLogger(t) zl := zerolog.New(nil).With().Logger() @@ -292,7 +290,7 @@ func TestRealFS_RemoveBinary(t *testing.T) { tt.args.binaryPath, tt.args.name, tt.args.verbose, - tt.args.logger(), // Call factory to get logger + tt.args.logger(t), // Call factory to get logger ) if (err != nil) != tt.wantErr { t.Errorf("RemoveBinary() error = %v, wantErr %v", err, tt.wantErr) diff --git a/internal/logger/logger.go b/internal/logger/logger.go index a3014ab..7fedc0b 100644 --- a/internal/logger/logger.go +++ b/internal/logger/logger.go @@ -315,9 +315,10 @@ func (z *ZerologLogger) SetCaptureFunc(captureFunc LogCaptureFunc) { z.captureFunc = captureFunc z.mu.Unlock() - // Set up the bridge function on the capture writer when a callback is provided. + // Set up the bridge function on the capture writer when it exists. // The bridge function reads the current captureFunc from the logger and calls it. - if z.captureWriter != nil && captureFunc != nil { + // This ensures capture is properly disabled when captureFunc is nil. + if z.captureWriter != nil { z.captureWriter.SetupCaptureBridge(func(level, msg string) { z.mu.RLock() capture := z.captureFunc From b91d891f9892fd0dce8d085908f974fbd286984d Mon Sep 17 00:00:00 2001 From: Nick Fedor Date: Mon, 2 Mar 2026 04:04:22 -0700 Subject: [PATCH 19/20] fix(logger): fix race condition and message extraction - Fix race condition in SetCaptureFunc by reordering handler updates - Correct log message extraction to use parsed parts instead of string indexing - Improve error handling in cli test helpers and fs test file operations - Update test mocks with proper zerolog initialization --- internal/cli/cli_test.go | 100 ++++++++++++++++++++++++++++++-------- internal/fs/fs_test.go | 8 ++- internal/logger/logger.go | 15 +++--- 3 files changed, 94 insertions(+), 29 deletions(-) diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index 36584a1..e914870 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -70,16 +70,34 @@ func captureStdout(t *testing.T) func() string { t.Helper() oldStdout := os.Stdout - r, w, _ := os.Pipe() + + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("Failed to create pipe: %v", err) + } + os.Stdout = w return func() string { + // Always restore os.Stdout first os.Stdout = oldStdout - w.Close() + // Close the write end and check for errors + if err := w.Close(); err != nil { + t.Errorf("Failed to close pipe writer: %v", err) + } + // Read from pipe and check for errors var buf bytes.Buffer - buf.ReadFrom(r) + + if _, err := buf.ReadFrom(r); err != nil { + t.Errorf("Failed to read from pipe: %v", err) + } + + // Close the read end to prevent FD leak + if err := r.Close(); err != nil { + t.Errorf("Failed to close pipe reader: %v", err) + } return buf.String() } @@ -322,9 +340,12 @@ func TestRun(t *testing.T) { //nolint:thelper // Subtest functions in table-driven tests require *testing.T parameter for mock constructors func TestRun_WithLoggerSync(t *testing.T) { tests := []struct { - name string - config Config - setupFS func(t *testing.T) *mockFS.MockFS + name string + config Config + setupFS func(t *testing.T) *mockFS.MockFS + setupLog func(t *testing.T) *mockLogger.MockLogger + wantErr bool + wantOutput string }{ { name: "sync called on success", @@ -337,6 +358,19 @@ func TestRun_WithLoggerSync(t *testing.T) { return m }, + setupLog: func(t *testing.T) *mockLogger.MockLogger { + m := mockLogger.NewMockLogger(t) + nopLog := zerolog.New(io.Discard) + m.On("Debug").Return(nopLog.Debug()).Maybe() + m.On("Info").Return(nopLog.Info()).Maybe() + m.On("Warn").Return(nopLog.Warn()).Maybe() + m.On("Error").Return(nopLog.Error()).Maybe() + m.On("Sync").Return(nil) + + return m + }, + wantErr: false, + wantOutput: "Successfully removed tool\n", }, { name: "sync called on error", @@ -347,33 +381,57 @@ func TestRun_WithLoggerSync(t *testing.T) { return m }, + setupLog: func(t *testing.T) *mockLogger.MockLogger { + m := mockLogger.NewMockLogger(t) + nopLog := zerolog.New(io.Discard) + m.On("Debug").Return(nopLog.Debug()).Maybe() + m.On("Info").Return(nopLog.Info()).Maybe() + m.On("Warn").Return(nopLog.Warn()).Maybe() + m.On("Error").Return(nopLog.Error()).Maybe() + m.On("Sync").Return(nil) + + return m + }, + wantErr: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - mockLog := mockLogger.NewMockLogger(t) - mockLog.On("Sync").Return(nil) + // Capture stdout for output verification. + getOutput := captureStdout(t) + // Set up dependencies. mockFSInstance := tt.setupFS(t) + mockLog := tt.setupLog(t) + deps := Dependencies{ FS: mockFSInstance, Logger: mockLog, } - binDir, err := deps.FS.DetermineBinDir(tt.config.Goroot) - if err != nil { - _ = deps.Logger.Sync() + // Execute the Run function and capture any errors. + err := Run(deps, tt.config) + + // Capture stdout output after execution. + gotOutput := getOutput() - return + // Verify error behavior matches expectations. + if (err != nil) != tt.wantErr { + t.Errorf("Run() error = %v, wantErr %v", err, tt.wantErr) } - binaryPath := deps.FS.AdjustBinaryPath(binDir, tt.config.Binary) - _ = deps.FS.RemoveBinary(binaryPath, tt.config.Binary, tt.config.Verbose, deps.Logger) - _ = deps.Logger.Sync() + // Verify stdout output for success cases. + if tt.wantOutput != "" && gotOutput != tt.wantOutput { + t.Errorf("Run() output = %q, want %q", gotOutput, tt.wantOutput) + } // Assert that Sync was called on the mock logger. mockLog.AssertCalled(t, "Sync") + + // Assert that all mock expectations were met. + mockFSInstance.AssertExpectations(t) + mockLog.AssertExpectations(t) }) } } @@ -386,7 +444,11 @@ func TestRun_VerboseMode(t *testing.T) { m.On("RemoveBinary", "/bin/vhs", "vhs", true, mock.Anything).Return(nil) mockLog := mockLogger.NewMockLogger(t) - // Level is optional since the test doesn't go through the full Run() path + nopLog := zerolog.New(io.Discard) + mockLog.On("Debug").Return(nopLog.Debug()).Maybe() + mockLog.On("Info").Return(nopLog.Info()).Maybe() + mockLog.On("Warn").Return(nopLog.Warn()).Maybe() + mockLog.On("Error").Return(nopLog.Error()).Maybe() mockLog.On("Level", mock.Anything).Return().Maybe() mockLog.On("Sync").Return(nil) @@ -399,10 +461,8 @@ func TestRun_VerboseMode(t *testing.T) { // Capture stdout to capture output. getOutput := captureStdout(t) - binDir, _ := deps.FS.DetermineBinDir(config.Goroot) - binaryPath := deps.FS.AdjustBinaryPath(binDir, config.Binary) - err := deps.FS.RemoveBinary(binaryPath, config.Binary, config.Verbose, deps.Logger) - _ = deps.Logger.Sync() + // Execute the Run function and capture any errors. + err := Run(deps, config) // Restore stdout and get captured output. gotOutput := getOutput() diff --git a/internal/fs/fs_test.go b/internal/fs/fs_test.go index a667589..23d4c68 100644 --- a/internal/fs/fs_test.go +++ b/internal/fs/fs_test.go @@ -311,11 +311,15 @@ func TestRealFS_RemoveBinary_VerboseLogging(t *testing.T) { tmpDir := t.TempDir() tmpFile := filepath.Join(tmpDir, "testbin") - os.WriteFile(tmpFile, []byte("test"), 0o755) + + err := os.WriteFile(tmpFile, []byte("test"), 0o600) + if err != nil { + t.Fatalf("Failed to create test file: %v", err) + } fs := &RealFS{} - err := fs.RemoveBinary(tmpFile, "testbin", true, log) + err = fs.RemoveBinary(tmpFile, "testbin", true, log) if err != nil { t.Errorf("RemoveBinary() unexpected error = %v", err) } diff --git a/internal/logger/logger.go b/internal/logger/logger.go index 7fedc0b..e466a1e 100644 --- a/internal/logger/logger.go +++ b/internal/logger/logger.go @@ -160,9 +160,8 @@ func (w *captureWriter) captureLogMessage(logLine string) { if levelIndex >= 0 { // Level found: message is everything after the level field. - // Reconstruct by finding the level in the original line and taking everything after it. - msgStart := strings.Index(logLine, level) + len(level) - msg = strings.TrimSpace(logLine[msgStart:]) + // Build message from the parsed parts slice to avoid matching earlier tokens. + msg = strings.TrimSpace(strings.Join(parts[levelIndex+1:], " ")) } else { // No known level found: use the whole line as the message. msg = strings.TrimSpace(logLine) @@ -315,10 +314,13 @@ func (z *ZerologLogger) SetCaptureFunc(captureFunc LogCaptureFunc) { z.captureFunc = captureFunc z.mu.Unlock() - // Set up the bridge function on the capture writer when it exists. - // The bridge function reads the current captureFunc from the logger and calls it. - // This ensures capture is properly disabled when captureFunc is nil. + // Update the capture writer's state atomically when it exists. + // SetCaptureFunc is called FIRST to update the writer's capture handler and + // enable flag. Then SetupCaptureBridge installs a bridge that delegates to + // the writer's capture handler. This ordering prevents a race condition where + // logs could be dropped between setting up the bridge and updating the handler. if z.captureWriter != nil { + z.captureWriter.SetCaptureFunc(captureFunc) z.captureWriter.SetupCaptureBridge(func(level, msg string) { z.mu.RLock() capture := z.captureFunc @@ -328,7 +330,6 @@ func (z *ZerologLogger) SetCaptureFunc(captureFunc LogCaptureFunc) { capture(level, msg) } }) - z.captureWriter.SetCaptureFunc(captureFunc) } } From b8bc2cd806233fcbee707c1b6bbafa5093c4c830 Mon Sep 17 00:00:00 2001 From: Nick Fedor Date: Mon, 2 Mar 2026 04:32:22 -0700 Subject: [PATCH 20/20] test(cli): refactor test structure and remove dead code - Extract test execution logic into runTestCase helper function - Remove unused logger import and verbose log level configuration - Introduce testCase struct for standardized test definitions - Add nolint annotations for anonymous setup functions --- internal/cli/cli_test.go | 128 ++++++++++++++++++++------------------- 1 file changed, 65 insertions(+), 63 deletions(-) diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index e914870..aa2eff2 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -32,7 +32,6 @@ import ( mockRunner "github.com/nicholas-fedor/go-remove/internal/cli/mocks" mockFS "github.com/nicholas-fedor/go-remove/internal/fs/mocks" - "github.com/nicholas-fedor/go-remove/internal/logger" mockLogger "github.com/nicholas-fedor/go-remove/internal/logger/mocks" ) @@ -137,12 +136,6 @@ func makeDeps( func executeRun(deps Dependencies, config Config, runner ProgramRunner) error { log := deps.Logger - // Set log level based on config if verbose mode is enabled. - if config.Verbose { - level := logger.ParseLevel(config.LogLevel) - log.Level(level) - } - binDir, err := deps.FS.DetermineBinDir(config.Goroot) if err != nil { _ = log.Sync() // Flush logs; errors are ignored @@ -172,23 +165,65 @@ func executeRun(deps Dependencies, config Config, runner ProgramRunner) error { return nil } +// testCase defines the structure for TestRun test cases. +type testCase struct { + name string + config Config + setupFS func(t *testing.T) *mockFS.MockFS + setupLog func(t *testing.T) *mockLogger.MockLogger + setupRunner func(t *testing.T) *mockRunner.MockProgramRunner + wantErr bool + wantOutput string // Expected stdout output for non-verbose success +} + +// runTestCase executes a single test case with the provided configuration. +// It handles stdout capture, dependency setup, execution, and assertions. +func runTestCase(t *testing.T, tt *testCase) { + t.Helper() + + // Capture stdout for output verification. + getOutput := captureStdout(t) + + // Set up dependencies and runner. + deps, mockFSInstance, mockLog, runner := makeDeps(t, makeDepsConfig{ + setupFS: tt.setupFS, + setupLog: tt.setupLog, + setupRunner: tt.setupRunner, + }) + + // Execute the run function and capture any errors. + err := executeRun(deps, tt.config, runner) + + // Capture stdout output after execution. + gotOutput := getOutput() + + // Verify error behavior matches expectations. + if (err != nil) != tt.wantErr { + t.Errorf("Run() error = %v, wantErr %v", err, tt.wantErr) + } + + // Verify stdout output for non-verbose success cases. + if tt.wantOutput != "" && gotOutput != tt.wantOutput { + t.Errorf("Run() output = %q, want %q", gotOutput, tt.wantOutput) + } + + // Assert that all mock expectations were met. + mockFSInstance.AssertExpectations(t) + mockLog.AssertExpectations(t) + + // If using a mock runner, assert its expectations as well. + if mr, ok := runner.(*mockRunner.MockProgramRunner); ok { + mr.AssertExpectations(t) + } +} + // TestRun verifies the Run function's behavior under various conditions. -// -//nolint:thelper // Subtest functions in table-driven tests require *testing.T parameter for mock constructors func TestRun(t *testing.T) { - tests := []struct { - name string - config Config - setupFS func(t *testing.T) *mockFS.MockFS - setupLog func(t *testing.T) *mockLogger.MockLogger - setupRunner func(t *testing.T) *mockRunner.MockProgramRunner - wantErr bool - wantOutput string // Expected stdout output for non-verbose success - }{ + tests := []testCase{ { name: "direct removal success", config: Config{Binary: "vhs", Verbose: false, Goroot: false}, - setupFS: func(t *testing.T) *mockFS.MockFS { + setupFS: func(t *testing.T) *mockFS.MockFS { //nolint:thelper // Anonymous setup function, not a test helper m := mockFS.NewMockFS(t) m.On("DetermineBinDir", false).Return("/bin", nil) m.On("AdjustBinaryPath", "/bin", "vhs").Return("/bin/vhs") @@ -203,7 +238,7 @@ func TestRun(t *testing.T) { { name: "direct removal failure", config: Config{Binary: "vhs", Verbose: false, Goroot: false}, - setupFS: func(t *testing.T) *mockFS.MockFS { + setupFS: func(t *testing.T) *mockFS.MockFS { //nolint:thelper // Anonymous setup function, not a test helper m := mockFS.NewMockFS(t) m.On("DetermineBinDir", false).Return("/bin", nil) m.On("AdjustBinaryPath", "/bin", "vhs").Return("/bin/vhs") @@ -218,7 +253,7 @@ func TestRun(t *testing.T) { { name: "tui mode success", config: Config{Binary: "", Verbose: false, Goroot: false}, - setupFS: func(t *testing.T) *mockFS.MockFS { + setupFS: func(t *testing.T) *mockFS.MockFS { //nolint:thelper // Anonymous setup function, not a test helper m := mockFS.NewMockFS(t) m.On("DetermineBinDir", false).Return("/bin", nil) m.On("ListBinaries", "/bin").Return([]string{"vhs"}) @@ -226,7 +261,7 @@ func TestRun(t *testing.T) { return m }, setupLog: newMockLoggerWithDefaults, - setupRunner: func(t *testing.T) *mockRunner.MockProgramRunner { + setupRunner: func(t *testing.T) *mockRunner.MockProgramRunner { //nolint:thelper // Anonymous setup function, not a test helper m := mockRunner.NewMockProgramRunner(t) m.On("RunProgram", mock.Anything, mock.Anything).Return(nil, nil) @@ -237,7 +272,7 @@ func TestRun(t *testing.T) { { name: "tui mode no binaries", config: Config{Binary: "", Verbose: false, Goroot: false}, - setupFS: func(t *testing.T) *mockFS.MockFS { + setupFS: func(t *testing.T) *mockFS.MockFS { //nolint:thelper // Anonymous setup function, not a test helper m := mockFS.NewMockFS(t) m.On("DetermineBinDir", false).Return("/bin", nil) m.On("ListBinaries", "/bin").Return([]string{}) @@ -245,7 +280,7 @@ func TestRun(t *testing.T) { return m }, setupLog: newMockLoggerWithDefaults, - setupRunner: func(t *testing.T) *mockRunner.MockProgramRunner { + setupRunner: func(t *testing.T) *mockRunner.MockProgramRunner { //nolint:thelper // Anonymous setup function, not a test helper m := mockRunner.NewMockProgramRunner(t) // RunProgram may not be called if RunTUI returns an error early m.On("RunProgram", mock.Anything, mock.Anything).Return(nil, nil).Maybe() @@ -257,7 +292,7 @@ func TestRun(t *testing.T) { { name: "bin dir error", config: Config{Binary: "vhs", Verbose: false, Goroot: false}, - setupFS: func(t *testing.T) *mockFS.MockFS { + setupFS: func(t *testing.T) *mockFS.MockFS { //nolint:thelper // Anonymous setup function, not a test helper m := mockFS.NewMockFS(t) m.On("DetermineBinDir", false).Return("", errors.New("bin dir failed")) @@ -269,7 +304,7 @@ func TestRun(t *testing.T) { { name: "logger sync error", config: Config{Binary: "vhs", Verbose: false, Goroot: false}, - setupFS: func(t *testing.T) *mockFS.MockFS { + setupFS: func(t *testing.T) *mockFS.MockFS { //nolint:thelper // Anonymous setup function, not a test helper m := mockFS.NewMockFS(t) m.On("DetermineBinDir", false).Return("/bin", nil) m.On("AdjustBinaryPath", "/bin", "vhs").Return("/bin/vhs") @@ -277,7 +312,7 @@ func TestRun(t *testing.T) { return m }, - setupLog: func(t *testing.T) *mockLogger.MockLogger { + setupLog: func(t *testing.T) *mockLogger.MockLogger { //nolint:thelper // Anonymous setup function, not a test helper m := mockLogger.NewMockLogger(t) nopLog := zerolog.New(io.Discard) m.On("Debug").Return(nopLog.Debug()).Maybe() @@ -295,42 +330,9 @@ func TestRun(t *testing.T) { }, } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // Capture stdout for output verification. - getOutput := captureStdout(t) - - // Set up dependencies and runner. - deps, mockFSInstance, mockLog, runner := makeDeps(t, makeDepsConfig{ - setupFS: tt.setupFS, - setupLog: tt.setupLog, - setupRunner: tt.setupRunner, - }) - - // Execute the run function and capture any errors. - err := executeRun(deps, tt.config, runner) - - // Capture stdout output after execution. - gotOutput := getOutput() - - // Verify error behavior matches expectations. - if (err != nil) != tt.wantErr { - t.Errorf("Run() error = %v, wantErr %v", err, tt.wantErr) - } - - // Verify stdout output for non-verbose success cases. - if tt.wantOutput != "" && gotOutput != tt.wantOutput { - t.Errorf("Run() output = %q, want %q", gotOutput, tt.wantOutput) - } - - // Assert that all mock expectations were met. - mockFSInstance.AssertExpectations(t) - mockLog.AssertExpectations(t) - - // If using a mock runner, assert its expectations as well. - if mr, ok := runner.(*mockRunner.MockProgramRunner); ok { - mr.AssertExpectations(t) - } + for i := range tests { + t.Run(tests[i].name, func(t *testing.T) { + runTestCase(t, &tests[i]) }) } }