From baa95739fe3fa40751a705f581baa2248526b8e8 Mon Sep 17 00:00:00 2001 From: Alexander Adam Date: Sat, 2 Aug 2025 10:28:37 +0200 Subject: [PATCH 1/4] Improve error handling, add nil writer check, and enhance test coverage. - Introduced `ErrWriterNil` for handling `nil` writer cases. - Enhanced `PrintSimpleWriter` and `PrintLogoWriter` with additional validations. - Improved test coverage with new test cases for `BrokenNIO` scenarios. - Added `t.Parallel()` to applicable test cases for better concurrency. --- geany.go | 57 ++++++++++++++++++++++++++++-------------- geany_internal_test.go | 4 +++ geany_test.go | 42 +++++++++++++++++++++++++++---- 3 files changed, 79 insertions(+), 24 deletions(-) diff --git a/geany.go b/geany.go index 5ddf09a..9c45685 100644 --- a/geany.go +++ b/geany.go @@ -14,7 +14,10 @@ import ( "text/template" ) -// geanyData contains the data that geany provies on itself. +// ErrWriterNil indicates that the provided io.Writer is nil and cannot be used. +var ErrWriterNil = errors.New("writer is nil") + +// geanyData contains the data that geany provides on itself. type geanyData struct { VcsRevision string VcsTime string @@ -24,12 +27,15 @@ type geanyData struct { // logoData is the structure given as data to the templating engine. // It contains the geany provided build information and the user provided data. +// The user is not required to pass data, in that case nil should be passed. In any case, +// the data accessed in the logo template and provided data must match. type logoData struct { - Geany geanyData - Values any + Geany geanyData // build information + Values any // user provided data } -// getBuildInfo is a global variable to mock it easily in tests. +// getBuildInfo is a global variable to mock it easily in tests. Take into account that +// this variable can produce race conditions in parallel testing. var getBuildInfo = debug.ReadBuildInfo //nolint:gochecknoglobals // prepareLogoData collects the build information and the user-provided data @@ -45,7 +51,7 @@ func prepareLogoData(values any) logoData { Values: values, } - if buildInfo, ok := getBuildInfo(); ok { + if buildInfo, ok := getBuildInfo(); ok && buildInfo != nil { result.Geany.GoVersion = buildInfo.GoVersion for _, s := range buildInfo.Settings { @@ -68,24 +74,28 @@ func prepareLogoData(values any) logoData { // PrintSimpleWriter outputs just the name of the program, the build information // and, in case, the user given data to a user provided io.Writer. func PrintSimpleWriter(writer io.Writer, values any) error { + if writer == nil { + return ErrWriterNil + } + revData := prepareLogoData(values) // normally we have the program's name given as the first argument - if len(os.Args) > 0 { - fmt.Printf("%s\n", os.Args[0]) + if len(os.Args) > 0 && os.Args[0] != "" { + if _, err := fmt.Fprintf(writer, "%s\n", os.Args[0]); err != nil { + return fmt.Errorf("could not write: %w", err) + } } encoder := json.NewEncoder(writer) encoder.SetIndent("", " ") // we suppress the linter here, as we cannot guarantee for users data. - err := encoder.Encode(revData) //nolint:musttag - - if err == nil { - _, err = fmt.Fprintln(writer) + if err := encoder.Encode(revData); err != nil { //nolint:musttag + return fmt.Errorf("could not encode user data: %w", err) } - if err != nil { + if _, err := fmt.Fprintln(writer); err != nil { return fmt.Errorf("could not write: %w", err) } @@ -105,23 +115,32 @@ func PrintSimple(values any) error { // - VcsModified // - GoVersion // -// these can be referenced in the template, e.g., using {{ .VcsRevision }}. +// these can be referenced in the template, e.g., using `{{ .Geany.VcsRevision }}`. // An additional custom value can be accessed via the Values field. Its type must match the way -// that it is accessed in the logo. +// that it is accessed in the logo and is accessed using e.g. `{{ .Values.Foo }}`. +// +// The template is parsed and executed. In case of an error, the program's name and user given +// data are printed as JSON as fallback. The original error and, in case, the error of the fallback +// are returned. func PrintLogoWriter(writer io.Writer, tmpl string, values any) error { + if writer == nil { + return ErrWriterNil + } + revData := prepareLogoData(values) logo := template.New("logo") - template.Must(logo.Parse(tmpl)) + + if _, err := logo.Parse(tmpl); err != nil { + return fmt.Errorf("could not parse template: %w", err) + } if err := logo.Execute(writer, revData); err != nil { return errors.Join(err, PrintSimpleWriter(writer, values)) } - _, err := fmt.Fprintln(writer) - - if err != nil { - return fmt.Errorf("could not write: %w", err) + if _, err := fmt.Fprintln(writer); err != nil { + return fmt.Errorf("could not write final newline: %w", err) } return nil diff --git a/geany_internal_test.go b/geany_internal_test.go index 2c6a03b..def5f48 100644 --- a/geany_internal_test.go +++ b/geany_internal_test.go @@ -20,6 +20,8 @@ func Must[T any](t T, err error) T { } func TestPrepareLogoDataNormal(t *testing.T) { + t.Parallel() + logoData := prepareLogoData(nil) buildInfo, ok := debug.ReadBuildInfo() @@ -34,6 +36,8 @@ func TestPrepareLogoDataNormal(t *testing.T) { } func TestPrepareLogoDataMocked(t *testing.T) { + t.Parallel() + old := getBuildInfo getBuildInfo = func() (*debug.BuildInfo, bool) { result := debug.BuildInfo{ diff --git a/geany_test.go b/geany_test.go index e60fa33..0132fd4 100644 --- a/geany_test.go +++ b/geany_test.go @@ -104,6 +104,8 @@ func (b *BrokenNIO) Write(in []byte) (n int, err error) { } func TestBrokenLogoWriter(t *testing.T) { + t.Parallel() + target := BrokenNIO{} err := geany.PrintLogoWriter( @@ -116,6 +118,8 @@ func TestBrokenLogoWriter(t *testing.T) { } func TestBrokenLogoWriterFallback(t *testing.T) { + t.Parallel() + target := BrokenNIO{To: 1} err := geany.PrintLogoWriter( @@ -128,6 +132,8 @@ func TestBrokenLogoWriterFallback(t *testing.T) { } func TestBrokenLogoWriterAtEnd(t *testing.T) { + t.Parallel() + target := BrokenNIO{From: 2} err := geany.PrintLogoWriter( @@ -140,6 +146,8 @@ func TestBrokenLogoWriterAtEnd(t *testing.T) { } func TestBrokenSimpleWriter(t *testing.T) { + t.Parallel() + target := BrokenNIO{} require.Error(t, @@ -149,10 +157,34 @@ func TestBrokenSimpleWriter(t *testing.T) { "simple writer no printing error") } +func TestBrokenSimpleWriterUserData(t *testing.T) { + t.Parallel() + + target := BrokenNIO{From: 1} + + require.Error(t, + geany.PrintSimpleWriter( + &target, + nil), + "simple writer no printing error") +} + +func TestBrokenSimpleWriterFinal(t *testing.T) { + t.Parallel() + + target := BrokenNIO{From: 2} + + require.Error(t, + geany.PrintSimpleWriter( + &target, + nil), + "simple writer no printing error") +} + func TestBrokenLogo(t *testing.T) { - assert.Panics(t, - func() { - _ = geany.PrintLogo("{{ .Geany }", nil) - }, - "broken logo does not panic") + t.Parallel() + + require.Error(t, + geany.PrintLogo("{{ .Geany }", nil), + "broken logo does produce error") } From 9434695af0d0f36fa8a42a740e6233d0496f47b3 Mon Sep 17 00:00:00 2001 From: Alexander Adam Date: Sat, 2 Aug 2025 10:32:37 +0200 Subject: [PATCH 2/4] Update changelog. --- CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ed39a8..4cf8606 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,11 @@ +Release 0.1.2 +============= + +- introduced ErrWriterNil to catch nil writers +- better parameter validation +- improved tests with configurable broken IO +- added parallel annotation + Release 0.1.1 ============= From cfedacb27f3a577075d99a536f94ce58b222ed03 Mon Sep 17 00:00:00 2001 From: Alexander Adam Date: Sat, 2 Aug 2025 10:47:11 +0200 Subject: [PATCH 3/4] Refine error messages for clarity and update associated tests - Updated `fmt.Errorf` calls to provide more descriptive error messages. - Modified relevant error assertions in tests to align with the updated error messages. - Added a changelog entry for improved error message clarity. --- CHANGELOG.md | 1 + geany.go | 4 ++-- geany_test.go | 7 +++++-- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4cf8606..74379d4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ Release 0.1.2 - introduced ErrWriterNil to catch nil writers - better parameter validation +- better error messages - improved tests with configurable broken IO - added parallel annotation diff --git a/geany.go b/geany.go index 9c45685..ac58d38 100644 --- a/geany.go +++ b/geany.go @@ -83,7 +83,7 @@ func PrintSimpleWriter(writer io.Writer, values any) error { // normally we have the program's name given as the first argument if len(os.Args) > 0 && os.Args[0] != "" { if _, err := fmt.Fprintf(writer, "%s\n", os.Args[0]); err != nil { - return fmt.Errorf("could not write: %w", err) + return fmt.Errorf("could not write program name: %w", err) } } @@ -96,7 +96,7 @@ func PrintSimpleWriter(writer io.Writer, values any) error { } if _, err := fmt.Fprintln(writer); err != nil { - return fmt.Errorf("could not write: %w", err) + return fmt.Errorf("could not write final newline: %w", err) } return nil diff --git a/geany_test.go b/geany_test.go index 0132fd4..93aa009 100644 --- a/geany_test.go +++ b/geany_test.go @@ -114,7 +114,10 @@ func TestBrokenLogoWriter(t *testing.T) { nil) require.Error(t, err, "simple writer printing error") - require.Equal(t, "broken writer\ncould not write: broken writer", err.Error(), "not two broken writer errors") + require.Equal(t, + "broken writer\ncould not write program name: broken writer", + err.Error(), + "not two broken writer errors") } func TestBrokenLogoWriterFallback(t *testing.T) { @@ -142,7 +145,7 @@ func TestBrokenLogoWriterAtEnd(t *testing.T) { nil) require.Error(t, err, "simple writer printing error") - assert.Equal(t, "could not write: broken writer", err.Error(), "just one broken writer error") + assert.Equal(t, "could not write final newline: broken writer", err.Error(), "just one broken writer error") } func TestBrokenSimpleWriter(t *testing.T) { From 918a3027b66af321935cf1c38a849657d904780f Mon Sep 17 00:00:00 2001 From: Alexander Adam Date: Sat, 2 Aug 2025 18:11:24 +0200 Subject: [PATCH 4/4] Add nil writer test cases to enhance error validation - Introduced `TestLogoWriterNil` to verify behavior when writer is `nil`. - Added `t.Parallel()` for better test concurrency. --- geany_test.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/geany_test.go b/geany_test.go index 93aa009..6b81893 100644 --- a/geany_test.go +++ b/geany_test.go @@ -191,3 +191,17 @@ func TestBrokenLogo(t *testing.T) { geany.PrintLogo("{{ .Geany }", nil), "broken logo does produce error") } + +func TestLogoWriterNil(t *testing.T) { + t.Parallel() + + require.ErrorIs(t, + geany.PrintLogoWriter(nil, "", nil), + geany.ErrWriterNil, + "nil writer does not produce error") + + require.ErrorIs(t, + geany.PrintSimpleWriter(nil, nil), + geany.ErrWriterNil, + "nil writer does not produce error") +}