Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SMTP client tests overhaul #361

Merged
merged 48 commits into from
Nov 11, 2024
Merged

SMTP client tests overhaul #361

merged 48 commits into from
Nov 11, 2024

Conversation

wneessen
Copy link
Owner

@wneessen wneessen commented Nov 11, 2024

This PR improves the overall test suite of go-mail/smtp. Most of the tests have been reworked and improved for code readability and maintainability.

The testing is now more structured and standarized. The coverage is now +95% for all files.

During the refactoring the following bugs were found and fixed:

  • Fix typo in error message in normalizeString function d6f256c
  • Remove redundant empty string check in SCRAM normalization a1efa1a
  • Add nil check in UpdateDeadline method 8fbd94a
  • Lock mutex when setting the logger 9412f31

This test ensures that when MessageID is called on a nil SendError, it returns an empty string. This additional check helps verify the correct behavior of the MessageID method under nil conditions.
Improved the structure and readability of the authentication tests by using subtests for each scenario, ensuring better isolation and clearer failure reporting. Removed unnecessary imports and redundant code, reducing complexity and enhancing maintainability.
Refactor the test function `TestPlainAuth_noEnc` to include subtests for better organization and add more comprehensive error handling. This improves clarity and robustness by verifying various authentication scenarios and expected outcomes.
Updated the parameter name `allowUnEnc` to `allowUnenc` in both `LoginAuth` and `PlainAuth` functions to maintain consistent naming conventions. This change improves code readability and follows standard naming practices.
Rename and uncomment TestLoginAuth with more test cases, ensuring coverage for successful and failed authentication scenarios, including checks for unencrypted logins and server response errors. This improves test robustness and coverage.
Renamed the test functions and improved the test structure for login authentication checks. Added subtests to provide clear descriptions and enhance error checking.
Added a simple SMTP test server with basic features like PLAIN, LOGIN, and NOENC authentication. It can start, handle connections, and simulate authentication success or failure. Included support for TLS with a generated localhost certificate.
This commit enhances the cleanup process in the SMTP tests by adding t.Cleanup to close client connections. Additionally, it rewrites the TestXOAuth2 function to include more detailed sub-tests, enhancing test granularity and readability.
Introduces two tests for XOAuth2 authentication in the SMTP package. The first test ensures successful authentication with valid credentials, while the second test verifies that authentication fails with incorrect settings.
Added comprehensive unit tests for SCRAM-SHA-1, SCRAM-SHA-256, and their PLUS variants. Implemented a test server to simulate various SCRAM authentication scenarios and validate both success and failure cases.
Corrected the spelling of "failed" in the error handling branch of the normalizeString function within smtp/auth_scram.go. This change addresses a minor typographical error to ensure the error message is clear and accurate.
The existing check for an empty normalized string is unnecessary because the OpaqueString profile in precis already throws an error if an empty string is returned: https://cs.opensource.google/go/x/text/+/master:secure/precis/profiles.go;l=66
Corrected "crypto/tl" to "crypto/tls" in the comment for better clarity and accuracy. This typo fix ensures that the code comments correctly reference the relevant Go package.
Introduce new test cases to validate the error handling of the scramAuth methods. These tests cover scenarios such as invalid characters in usernames, empty inputs, and edge cases like broken rand.Reader.
Consolidate and organize SMTP test cases by removing obsolete tests and adding focused, detailed tests for CRAM-MD5 and new client behaviors. This ensures clearer test structure and better coverage of edge cases.
Add tests to ensure HELO/EHLO commands fail with empty name, newline in name, and double execution. This improves validation and robustness of the SMTP client implementation.
This commit introduces a new test case for the `Client`'s `cmd` method to ensure it fails correctly when the `textproto` command encounters a broken writer. It also adds a `failWriter` struct that simulates a write failure to facilitate this testing scenario.
Introduce new tests to cover the Client's behavior when initiating a STARTTLS session under different conditions: normal operation, failure on EHLO/HELO, and a server not supporting STARTTLS. This ensures robustness in handling STARTTLS interactions.
Introduce tests to verify TLS connection state handling in the SMTP client. Ensure that normal TLS connections return a valid state, and non-TLS connections do not wrongly indicate a TLS state.
Introduce new tests for the SMTP client covering scenarios for Mail, Verify, and Auth commands to ensure correct behavior under various conditions. Updated `simpleSMTPServer` implementation to handle more cases including VRFY and SMTPUTF8.
Introduce new test cases to verify the SMTP server's ability to handle DSN, 8BITMIME, and SMTPUTF8 features. These tests ensure correct response behavior when these features are supported by the server.
Update SMTP tests to use t.Cleanup for client cleanup to ensure proper resource release. Introduce a new test, TestClient_Rcpt, to verify recipient address handling under various conditions.
Changed `t.Errorf` to `t.Fatalf` in multiple instances within the test cases. This ensures that the tests halt immediately upon a critical failure, improving test reliability and debugging clarity.
Replace repetitive TLS configuration code with a reusable `getTLSConfig` helper function for consistency and maintainability. Additionally, update port configuration and add new tests for mail data transmission.
The testHookStartTLS variable and its related conditional code have been removed from the smtp.go file. This cleanup streamlines the TLS initiation process and removes unnecessary test-specific hooks no longer in use.
Refactored SMTP server tests to use an echo buffer for capturing responses. This allows for better validation of command responses without relying on error messages. Additionally, added a new test case to validate a full SendMail transaction with TLS and authentication.
Corrected indentation inconsistencies in the smtp_test.go file. Added multiple test cases to verify the failure scenarios for `SendMail` under various conditions including invalid hostnames, newlines in the address fields, and server failures during EHLO/HELO, STARTTLS, and authentication stages.
This commit introduces multiple test cases to handle SMTP failure scenarios such as invalid host, newline in addresses, EHLO/HELO failures, and malformed data injections. Additionally, it includes improvements in error handling for these specific conditions.
Introduced new unit tests to verify the SMTP client's behavior with extensions, reset, and noop commands under various server conditions. Updated server response handling to correctly manage feature sets when they are empty.
Deleted multiple outdated and redundant tests from the SMTP client test suite to streamline and improve the maintainability of the test codebase. This change focuses on removing tests that are no longer relevant or have been superseded by other methods.
These tests verify the behavior of the SetDebugLog method in various scenarios such as enabling and disabling debug logging and ensuring the logger type is as expected. This improves the robustness and reliability of the debug logging functionality in the Client class.
Add mutex locking to ensure thread-safety when setting the logger in the `smtp` package. This prevents potential race conditions and ensures that the logger is updated consistently in concurrent operations.
This commit introduces unit tests for the Client's SetLogger method. It verifies the correct logger type is set and ensures setting a nil logger does not override an existing logger.
Ensure that the connection is not nil before setting the deadline in the UpdateDeadline method. This prevents a potential runtime panic when attempting to set a deadline on a nil connection.
Introduce new tests to verify behaviors of client methods including SetLogAuthData, SetDSNRcptNotifyOption, SetDSNMailReturnOption, HasConnection, and UpdateDeadline. Ensure these tests cover various scenarios such as successful operations, edge cases, and failure conditions.
This commit introduces four test cases for the GetTLSConnectionState method. These tests cover scenarios with a valid TLS connection, no connection, a non-TLS connection, and a non-TLS connection with the TLS flag set on the client. This ensures comprehensive validation of the method's behavior across different states.
Introduce unit tests to verify the behavior of the debug logging in the Client. Confirm that logs are correctly produced when debug mode is enabled and appropriately suppressed when it is disabled.
Copy link

codecov bot commented Nov 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.57%. Comparing base (de3493e) to head (800c266).
Report is 49 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #361      +/-   ##
==========================================
+ Coverage   91.74%   96.57%   +4.82%     
==========================================
  Files          27       27              
  Lines        2944     2946       +2     
==========================================
+ Hits         2701     2845     +144     
+ Misses        172       70     -102     
+ Partials       71       31      -40     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Removed unused variables and improved error handling in smtp_test.go. Adjusted to capture only error in auth.Next() calls, ensuring accurate validation. Added necessary error checks after creating new client connections to prevent test failures.
Changed variable declarations from '=' to ':=' to properly handle errors within the SMTP test cases. This ensures that errors are correctly captured and reported when writing to the EchoBuffer.
Introduced a mutex to the SMTP test server properties to ensure thread-safe access when handling connections. This prevents race conditions and improves the reliability of the test server under concurrent load.
Removed redundant mutex and streamlined anonymous goroutine syntax for test server setup by passing echoBuffer directly as a parameter. This change reduces unnecessary use of shared resources and simplifies the test code structure and fixes potential race conditions
Removed JSON logger tests from smtp_test.go and relocated them to a new file smtp_121_test.go, ensuring compliance with Go 1.21. This change maintains test integrity while organizing tests by Go version compatibility.
Renamed 'err' to 'berr' to avoid shadowing outer variable. This change ensures clearer error handling and avoids potential issues with variable scope and readability in the smtp_test.go file.
Moved serverProps outside goroutines to improve code readability and maintainability. Added a RWMutex to serverProps to ensure thread-safe access to EchoBuffer, preventing race conditions during concurrent writes.
@wneessen wneessen marked this pull request as ready for review November 11, 2024 18:57
@wneessen wneessen merged commit 580ef5e into main Nov 11, 2024
29 checks passed
@wneessen wneessen deleted the smtp-client-tests branch November 11, 2024 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant