From 27a39852408a219be409a82d8e07a631c14552fb Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Fri, 1 Nov 2024 15:24:47 +0100 Subject: [PATCH 1/5] Refactor and expand random string tests Refactored the test for `randomStringSecure` to better organize test cases using subtests. Added new test cases to check failures with a broken rand.Reader, improving test coverage and robustness. --- random_test.go | 86 +++++++++++++++++++++++++++++++++++--------------- 1 file changed, 61 insertions(+), 25 deletions(-) diff --git a/random_test.go b/random_test.go index a608c2a..b59eba6 100644 --- a/random_test.go +++ b/random_test.go @@ -5,45 +5,81 @@ package mail import ( + "crypto/rand" + "errors" "strings" "testing" ) // TestRandomStringSecure tests the randomStringSecure method func TestRandomStringSecure(t *testing.T) { - tt := []struct { - testName string - length int - mustNotMatch string - }{ - {"20 chars", 20, "'"}, - {"100 chars", 100, "'"}, - {"1000 chars", 1000, "'"}, - } + t.Run("randomStringSecure with varying length", func(t *testing.T) { + tt := []struct { + testName string + length int + mustNotMatch string + }{ + {"20 chars", 20, "'"}, + {"100 chars", 100, "'"}, + {"1000 chars", 1000, "'"}, + } - for _, tc := range tt { - t.Run(tc.testName, func(t *testing.T) { - rs, err := randomStringSecure(tc.length) - if err != nil { - t.Errorf("random string generation failed: %s", err) - } - if strings.Contains(rs, tc.mustNotMatch) { - t.Errorf("random string contains unexpected character. got: %s, not-expected: %s", - rs, tc.mustNotMatch) - } - if len(rs) != tc.length { - t.Errorf("random string length does not match. expected: %d, got: %d", tc.length, len(rs)) - } - }) - } + for _, tc := range tt { + t.Run(tc.testName, func(t *testing.T) { + rs, err := randomStringSecure(tc.length) + if err != nil { + t.Errorf("random string generation failed: %s", err) + } + if strings.Contains(rs, tc.mustNotMatch) { + t.Errorf("random string contains unexpected character. got: %s, not-expected: %s", + rs, tc.mustNotMatch) + } + if len(rs) != tc.length { + t.Errorf("random string length does not match. expected: %d, got: %d", tc.length, len(rs)) + } + }) + } + }) + t.Run("randomStringSecure fails on broken rand Reader (first read)", func(t *testing.T) { + defaultRandReader := rand.Reader + t.Cleanup(func() { rand.Reader = defaultRandReader }) + rand.Reader = &randReader{failon: 1} + if _, err := randomStringSecure(22); err == nil { + t.Fatalf("expected failure on broken rand Reader") + } + }) + t.Run("randomStringSecure fails on broken rand Reader (second read)", func(t *testing.T) { + defaultRandReader := rand.Reader + t.Cleanup(func() { rand.Reader = defaultRandReader }) + rand.Reader = &randReader{failon: 0} + if _, err := randomStringSecure(22); err == nil { + t.Fatalf("expected failure on broken rand Reader") + } + }) } func BenchmarkGenerator_RandomStringSecure(b *testing.B) { b.ReportAllocs() for i := 0; i < b.N; i++ { - _, err := randomStringSecure(22) + _, err := randomStringSecure(10) if err != nil { b.Errorf("RandomStringFromCharRange() failed: %s", err) } } } + +// randReader is type that satisfies the io.Reader interface. It can fail on a specific read +// operations and is therefore useful to test consecutive reads with errors +type randReader struct { + failon uint8 + call uint8 +} + +// Read implements the io.Reader interface for the randReader type +func (r *randReader) Read(p []byte) (int, error) { + if r.call == r.failon { + r.call++ + return len(p), nil + } + return 0, errors.New("broken reader") +} From 25b7f81e3bbbef64a9d66bc08cfa43e58ec106ce Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Fri, 1 Nov 2024 15:57:59 +0100 Subject: [PATCH 2/5] Refactor error handling logic and string formatting Replaced constant with named error for readability and maintainability in the error handling condition. Adjusted error message formatting by removing an extra space for consistency. --- senderror.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/senderror.go b/senderror.go index 1943e28..4471208 100644 --- a/senderror.go +++ b/senderror.go @@ -81,7 +81,7 @@ type SendErrReason int // Returns: // - A string representing the error message. func (e *SendError) Error() string { - if e.Reason > 10 { + if e.Reason > ErrAmbiguous { return "unknown reason" } @@ -93,7 +93,7 @@ func (e *SendError) Error() string { errMessage.WriteRune(' ') errMessage.WriteString(e.errlist[i].Error()) if i != len(e.errlist)-1 { - errMessage.WriteString(", ") + errMessage.WriteString(",") } } } From e37dd39654a90541486e0e1a520ade419220a6f3 Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Fri, 1 Nov 2024 15:58:11 +0100 Subject: [PATCH 3/5] Refactor `senderror_test.go` for improved test clarity Consolidated multiple duplicate test cases into grouped sub-tests with clear names. This enhances readability and maintainability, ensures proper test isolation, and removes redundant code. --- senderror_test.go | 306 ++++++++++++++++++++++++++-------------------- 1 file changed, 176 insertions(+), 130 deletions(-) diff --git a/senderror_test.go b/senderror_test.go index e04b7ee..3cd3a76 100644 --- a/senderror_test.go +++ b/senderror_test.go @@ -6,163 +6,210 @@ package mail import ( "errors" - "fmt" "strings" "testing" ) // TestSendError_Error tests the SendError and SendErrReason error handling methods func TestSendError_Error(t *testing.T) { - tl := []struct { - n string - r SendErrReason - te bool - }{ - {"ErrGetSender/temp", ErrGetSender, true}, - {"ErrGetSender/perm", ErrGetSender, false}, - {"ErrGetRcpts/temp", ErrGetRcpts, true}, - {"ErrGetRcpts/perm", ErrGetRcpts, false}, - {"ErrSMTPMailFrom/temp", ErrSMTPMailFrom, true}, - {"ErrSMTPMailFrom/perm", ErrSMTPMailFrom, false}, - {"ErrSMTPRcptTo/temp", ErrSMTPRcptTo, true}, - {"ErrSMTPRcptTo/perm", ErrSMTPRcptTo, false}, - {"ErrSMTPData/temp", ErrSMTPData, true}, - {"ErrSMTPData/perm", ErrSMTPData, false}, - {"ErrSMTPDataClose/temp", ErrSMTPDataClose, true}, - {"ErrSMTPDataClose/perm", ErrSMTPDataClose, false}, - {"ErrSMTPReset/temp", ErrSMTPReset, true}, - {"ErrSMTPReset/perm", ErrSMTPReset, false}, - {"ErrWriteContent/temp", ErrWriteContent, true}, - {"ErrWriteContent/perm", ErrWriteContent, false}, - {"ErrConnCheck/temp", ErrConnCheck, true}, - {"ErrConnCheck/perm", ErrConnCheck, false}, - {"ErrNoUnencoded/temp", ErrNoUnencoded, true}, - {"ErrNoUnencoded/perm", ErrNoUnencoded, false}, - {"ErrAmbiguous/temp", ErrAmbiguous, true}, - {"ErrAmbiguous/perm", ErrAmbiguous, false}, - {"Unknown/temp", 9999, true}, - {"Unknown/perm", 9999, false}, - } - - for _, tt := range tl { - t.Run(tt.n, func(t *testing.T) { - if err := returnSendError(tt.r, tt.te); err != nil { - exp := &SendError{Reason: tt.r, isTemp: tt.te} - if !errors.Is(err, exp) { - t.Errorf("error mismatch, expected: %s (temp: %t), got: %s (temp: %t)", tt.r, tt.te, - exp.Error(), exp.isTemp) + t.Run("TestSendError_Error with various reasons", func(t *testing.T) { + tests := []struct { + name string + reason SendErrReason + isTemp bool + }{ + {"ErrGetSender/temp", ErrGetSender, true}, + {"ErrGetSender/perm", ErrGetSender, false}, + {"ErrGetRcpts/temp", ErrGetRcpts, true}, + {"ErrGetRcpts/perm", ErrGetRcpts, false}, + {"ErrSMTPMailFrom/temp", ErrSMTPMailFrom, true}, + {"ErrSMTPMailFrom/perm", ErrSMTPMailFrom, false}, + {"ErrSMTPRcptTo/temp", ErrSMTPRcptTo, true}, + {"ErrSMTPRcptTo/perm", ErrSMTPRcptTo, false}, + {"ErrSMTPData/temp", ErrSMTPData, true}, + {"ErrSMTPData/perm", ErrSMTPData, false}, + {"ErrSMTPDataClose/temp", ErrSMTPDataClose, true}, + {"ErrSMTPDataClose/perm", ErrSMTPDataClose, false}, + {"ErrSMTPReset/temp", ErrSMTPReset, true}, + {"ErrSMTPReset/perm", ErrSMTPReset, false}, + {"ErrWriteContent/temp", ErrWriteContent, true}, + {"ErrWriteContent/perm", ErrWriteContent, false}, + {"ErrConnCheck/temp", ErrConnCheck, true}, + {"ErrConnCheck/perm", ErrConnCheck, false}, + {"ErrNoUnencoded/temp", ErrNoUnencoded, true}, + {"ErrNoUnencoded/perm", ErrNoUnencoded, false}, + {"ErrAmbiguous/temp", ErrAmbiguous, true}, + {"ErrAmbiguous/perm", ErrAmbiguous, false}, + {"Unknown/temp", 9999, true}, + {"Unknown/perm", 9999, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := returnSendError(tt.reason, tt.isTemp) + if err == nil { + t.Fatalf("error expected, got nil") } - if !strings.Contains(fmt.Sprintf("%s", err), tt.r.String()) { + want := &SendError{Reason: tt.reason, isTemp: tt.isTemp} + if !errors.Is(err, want) { + t.Errorf("error mismatch, expected: %s (temp: %t), got: %s (temp: %t)", + tt.reason, tt.isTemp, want.Error(), want.isTemp) + } + if !strings.Contains(err.Error(), tt.reason.String()) { t.Errorf("error string mismatch, expected: %s, got: %s", - tt.r.String(), fmt.Sprintf("%s", err)) + tt.reason.String(), err.Error()) } - } - }) - } + }) + } + }) + t.Run("TestSendError_Error with multiple errors", func(t *testing.T) { + message := testMessage(t) + err := &SendError{ + affectedMsg: message, + errlist: []error{ErrNoRcptAddresses, ErrNoFromAddress}, + rcpt: []string{"", ""}, + Reason: ErrAmbiguous, + } + if !strings.Contains(err.Error(), "ambiguous reason, check Msg.SendError for message specific reasons") { + t.Errorf("error string mismatch, expected: ambiguous reason, check Msg.SendError for message "+ + "specific reasons, got: %s", err.Error()) + } + if !strings.Contains(err.Error(), "no recipient addresses set, no FROM address set") { + t.Errorf("error string mismatch, expected: no recipient addresses set, no FROM address set, got: %s", + err.Error()) + } + if !strings.Contains(err.Error(), "affected recipient(s): , "+ + "") { + t.Errorf("error string mismatch, expected: affected recipient(s): , "+ + ", got: %s", err.Error()) + } + }) +} + +func TestSendError_Is(t *testing.T) { + t.Run("TestSendError_Is errors match", func(t *testing.T) { + err1 := returnSendError(ErrAmbiguous, false) + err2 := returnSendError(ErrAmbiguous, false) + if !errors.Is(err1, err2) { + t.Error("error mismatch, expected ErrAmbiguous to be equal to ErrAmbiguous") + } + }) + t.Run("TestSendError_Is errors mismatch", func(t *testing.T) { + err1 := returnSendError(ErrAmbiguous, false) + err2 := returnSendError(ErrSMTPMailFrom, false) + if errors.Is(err1, err2) { + t.Error("error mismatch, ErrAmbiguous should not be equal to ErrSMTPMailFrom") + } + }) + t.Run("TestSendError_Is on nil", func(t *testing.T) { + var err *SendError + if err.Is(ErrNoFromAddress) { + t.Error("expected false on nil-senderror") + } + }) } func TestSendError_IsTemp(t *testing.T) { - var se *SendError - err1 := returnSendError(ErrAmbiguous, true) - if !errors.As(err1, &se) { - t.Errorf("error mismatch, expected error to be of type *SendError") - return - } - if errors.As(err1, &se) && !se.IsTemp() { - t.Errorf("error mismatch, expected temporary error") - return - } - err2 := returnSendError(ErrAmbiguous, false) - if !errors.As(err2, &se) { - t.Errorf("error mismatch, expected error to be of type *SendError") - return - } - if errors.As(err2, &se) && se.IsTemp() { - t.Errorf("error mismatch, expected non-temporary error") - return - } -} - -func TestSendError_IsTempNil(t *testing.T) { - var se *SendError - if se.IsTemp() { - t.Error("expected false on nil-senderror") - } + t.Run("TestSendError_IsTemp is true", func(t *testing.T) { + err := returnSendError(ErrAmbiguous, true) + if err == nil { + t.Fatalf("error expected, got nil") + } + var sendErr *SendError + if !errors.As(err, &sendErr) { + t.Fatal("error expected to be of type *SendError") + } + if !sendErr.IsTemp() { + t.Errorf("expected temporary error, got: temperr: %t", sendErr.IsTemp()) + } + }) + t.Run("TestSendError_IsTemp is false", func(t *testing.T) { + err := returnSendError(ErrAmbiguous, false) + if err == nil { + t.Fatalf("error expected, got nil") + } + var sendErr *SendError + if !errors.As(err, &sendErr) { + t.Fatal("error expected to be of type *SendError") + } + if sendErr.IsTemp() { + t.Errorf("expected permanent error, got: temperr: %t", sendErr.IsTemp()) + } + }) + t.Run("TestSendError_IsTemp is nil", func(t *testing.T) { + var se *SendError + if se.IsTemp() { + t.Error("expected false on nil-senderror") + } + }) } func TestSendError_MessageID(t *testing.T) { - var se *SendError - err := returnSendError(ErrAmbiguous, false) - if !errors.As(err, &se) { - t.Errorf("error mismatch, expected error to be of type *SendError") - return - } - if errors.As(err, &se) { - if se.MessageID() == "" { - t.Errorf("sendError expected message-id, but got empty string") + t.Run("TestSendError_MessageID message ID is set", func(t *testing.T) { + var sendErr *SendError + err := returnSendError(ErrAmbiguous, false) + if !errors.As(err, &sendErr) { + t.Fatal("error mismatch, expected error to be of type *SendError") } - if !strings.EqualFold(se.MessageID(), "") { + if sendErr.MessageID() == "" { + t.Error("sendError expected message-id, but got empty string") + } + if !strings.EqualFold(sendErr.MessageID(), "") { t.Errorf("sendError message-id expected: %s, but got: %s", "", - se.MessageID()) + sendErr.MessageID()) } - } -} - -func TestSendError_MessageIDNil(t *testing.T) { - var se *SendError - if se.MessageID() != "" { - t.Error("expected empty string on nil-senderror") - } + }) + t.Run("TestSendError_MessageID message ID is not set", func(t *testing.T) { + var sendErr *SendError + message := testMessage(t) + err := &SendError{ + affectedMsg: message, + errlist: []error{ErrNoRcptAddresses}, + rcpt: []string{"", ""}, + Reason: ErrAmbiguous, + } + if !errors.As(err, &sendErr) { + t.Fatal("error mismatch, expected error to be of type *SendError") + } + if sendErr.MessageID() != "" { + t.Errorf("sendError expected empty message-id, got: %s", sendErr.MessageID()) + } + }) } func TestSendError_Msg(t *testing.T) { - var se *SendError - err := returnSendError(ErrAmbiguous, false) - if !errors.As(err, &se) { - t.Errorf("error mismatch, expected error to be of type *SendError") - return - } - if errors.As(err, &se) { - if se.Msg() == nil { - t.Errorf("sendError expected msg pointer, but got nil") + t.Run("TestSendError_Msg message is set", func(t *testing.T) { + var sendErr *SendError + err := returnSendError(ErrAmbiguous, false) + if !errors.As(err, &sendErr) { + t.Fatal("error mismatch, expected error to be of type *SendError") } - from := se.Msg().GetFromString() + msg := sendErr.Msg() + if msg == nil { + t.Fatalf("sendError expected msg pointer, but got nil") + } + from := msg.GetFromString() if len(from) == 0 { - t.Errorf("sendError expected msg from, but got empty string") - return + t.Fatal("sendError expected msg from, but got empty string") } if !strings.EqualFold(from[0], "") { t.Errorf("sendError message from expected: %s, but got: %s", "", from[0]) } - } -} - -func TestSendError_MsgNil(t *testing.T) { - var se *SendError - if se.Msg() != nil { - t.Error("expected nil on nil-senderror") - } -} - -func TestSendError_IsFail(t *testing.T) { - err1 := returnSendError(ErrAmbiguous, false) - err2 := returnSendError(ErrSMTPMailFrom, false) - if errors.Is(err1, err2) { - t.Errorf("error mismatch, ErrAmbiguous should not be equal to ErrSMTPMailFrom") - } -} - -func TestSendError_ErrorMulti(t *testing.T) { - expected := `ambiguous reason, check Msg.SendError for message specific reasons, ` + - `affected recipient(s): , ` - err := &SendError{ - Reason: ErrAmbiguous, isTemp: false, affectedMsg: nil, - rcpt: []string{"", ""}, - } - if err.Error() != expected { - t.Errorf("error mismatch, expected: %s, got: %s", expected, err.Error()) - } + }) + t.Run("TestSendError_Msg message is not set", func(t *testing.T) { + var sendErr *SendError + err := &SendError{ + errlist: []error{ErrNoRcptAddresses}, + rcpt: []string{"", ""}, + Reason: ErrAmbiguous, + } + if !errors.As(err, &sendErr) { + t.Fatal("error mismatch, expected error to be of type *SendError") + } + if sendErr.Msg() != nil { + t.Errorf("sendError expected nil msg pointer, got: %v", sendErr.Msg()) + } + }) } // returnSendError is a helper method to retunr a SendError with a specific reason @@ -173,6 +220,5 @@ func returnSendError(r SendErrReason, t bool) error { message.Subject("This is the subject") message.SetBodyString(TypeTextPlain, "This is the message body") message.SetMessageIDWithValue("this.is.a.message.id") - return &SendError{Reason: r, isTemp: t, affectedMsg: message} } From 0fcde10768d2cd1d0fbd6af3cc970a7956b60c21 Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Fri, 1 Nov 2024 16:33:48 +0100 Subject: [PATCH 4/5] Remove output redirection from sendmail install This change ensures that the output of the apt-get commands is no longer redirected to /dev/null. This aids in debugging by making command outputs visible in the CI logs. --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 37e0363..f0411d3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -50,7 +50,7 @@ jobs: check-latest: true - name: Install sendmail run: | - sudo apt-get -y update >/dev/null && sudo apt-get -y upgrade >/dev/null && sudo DEBIAN_FRONTEND=noninteractive apt-get -y install nullmailer >/dev/null && which sendmail + sudo apt-get -y update && sudo apt-get -y upgrade && sudo DEBIAN_FRONTEND=noninteractive apt-get -y install nullmailer && which sendmail - name: Run go test if: success() run: | From ec10e0b13289191b1dc7decc33bcc8e9c473b19b Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Fri, 1 Nov 2024 16:36:06 +0100 Subject: [PATCH 5/5] Remove redundant upgrade command in CI workflow The `sudo apt-get -y upgrade` command was removed from the CI workflow's "Install sendmail" step. This change simplifies the installation process by ensuring only the necessary updates and installations are performed, which can contribute to faster and more reliable CI runs. --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f0411d3..2901602 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -50,7 +50,7 @@ jobs: check-latest: true - name: Install sendmail run: | - sudo apt-get -y update && sudo apt-get -y upgrade && sudo DEBIAN_FRONTEND=noninteractive apt-get -y install nullmailer && which sendmail + sudo apt-get -y update && sudo DEBIAN_FRONTEND=noninteractive apt-get -y install nullmailer && which sendmail - name: Run go test if: success() run: |