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

chore(validator): enforcing commit message character limit to 50. #25

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions internal/validator/validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,27 @@ func checkMessageSubject(s *string) error {
return nil
}

/**
* checkMessageLength - Checks if the commit message is equal to or under 50 characters.
*
* This function ensures the commit message does not exceed 50 characters in length.
*
* Parameters:
* - l (*string): A pointer to the string representing the length
* of the commit message.
Comment on lines +125 to +126
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter name l is misleading, how about something like msg since it is synonymous to "commit message" which this function should be checking for its length? Update the docs once you have made the necessary changes as well.

* Returns:
* - error: Returns an error if the commit message is more than 50 characters.
* Returns nil if the message is within the valid length
*/

func checkMessageLength(l *string) error {
if len(*l) >= 50 {
return fmt.Errorf("commit message exceeds 50 characters, current length: %d",
len(*l))
}
return nil
}

/**
* ValidateMessage: Validate a "git-commit" message.
*
Expand All @@ -142,6 +163,11 @@ func ValidateMessage(message *parser.Message) (string, error) {
return "", fmt.Errorf("%s", err)
}

// Validate the commit message length
if err := checkMessageLength(&message.Description); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are supposed to check the entire "commit message" and not just the description of the message.

A commit message is structured as - <TYPE>[OPTIONAL SCOPE]: <DESCRIPTION> (see the summary of the Conventional Commits specifications). An example commit message is - "chore(parser): improve parsing performance of the message".

return "", fmt.Errorf("%s", err)
}

// Return a success message if the commit message validation was successful
return "valid commit message", nil
}
256 changes: 256 additions & 0 deletions internal/validator/validator_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,256 @@
package validator

import (
"fmt"
"testing"

"github.com/Weburz/crisp/internal/parser"
)

// TestCheckMessageType tests the checkMessageType function.
func TestCheckMessageType(t *testing.T) {
testCases := []struct {
name string
messageType string
expectedError string
}{
{
name: "Valid Type - feat",
messageType: "feat",
expectedError: "",
},
{
name: "Invalid Type - Uppercase",
messageType: "Feat",
expectedError: `invalid commit message casing, "Feat" should be "feat"`,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
err := checkMessageType(&tc.messageType)
if tc.expectedError == "" {
if err != nil {
t.Errorf("expected no error, got %v", err)
}
} else {
if err == nil || err.Error() != tc.expectedError {
t.Errorf("expected error %q, got %q", tc.expectedError, err)
}
}
})
}
}

// TestCheckMessageScope tests the checkMessageScope function.
func TestCheckMessageScope(t *testing.T) {
testCases := []struct {
name string
scope string
expectedError string
}{
{
name: "Valid Scope - Lowercase",
scope: "user",
expectedError: "",
},
{
name: "Valid Scope - Empty",
scope: "",
expectedError: "",
},
{
name: "Invalid Scope - Uppercase",
scope: "User",
expectedError: `invalid commit message scope casing, "User" ` +
`should be "user"`},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
err := checkMessageScope(&tc.scope)
if tc.expectedError == "" {
if err != nil {
t.Errorf("expected no error, got %v", err)
}
} else {
if err == nil || err.Error() != tc.expectedError {
t.Errorf("expected error %q, got %q", tc.expectedError, err)
}
}
})
}
}

// TestCheckMessageSubject tests the checkMessageSubject function.
func TestCheckMessageSubject(t *testing.T) {
testCases := []struct {
name string
subject string
expectedError string
}{
{
name: "Valid Subject - Lowercase",
subject: "add a new feature",
expectedError: "",
},
{
name: "Invalid Subject - Uppercase",
subject: "Add a new feature",
expectedError: "commit message subject should be lowercased & " +
"not end with a period(.)",
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
err := checkMessageSubject(&tc.subject)
if tc.expectedError == "" {
if err != nil {
t.Errorf("expected no error, got %v", err)
}
} else {
if err == nil || err.Error() != tc.expectedError {
t.Errorf("expected error %q, got %q", tc.expectedError, err)
}
}
})
}
}

// TestCheckMessageLength tests the checkMessageLength function.
func TestCheckMessageLength(t *testing.T) {
testCases := []struct {
name string
message string
expectedError string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The expectedError can be of the error interface type instead of the generic string interface type no?

}{
{
name: "Valid Length",
message: "feat: add a new feature",
expectedError: "",
},
{
name: "Invalid Length",
message: "Add a new feature that makes the application run " +
"faster and more efficiently",
expectedError: fmt.Sprintf(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the expectedError is of the error interface type then you can use the fmt.Errorf() function to define an error message as well.

"commit message exceeds 50 characters, current length: %d",
len("Add a new feature that makes the application run "+
"faster and more efficiently"),
),
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
err := checkMessageLength(&tc.message)
if tc.expectedError == "" {
if err != nil {
t.Errorf("expected no error, got %v", err)
}
} else {
if err == nil || err.Error() != tc.expectedError {
t.Errorf("expected error %q, got %q", tc.expectedError, err)
}
Comment on lines +153 to +155
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The conditional check for the error message can be much simplified if the expectedError is of the error interface type, so refactor it after a thorough evaluation of the aforementioned comments.

}
})
}
}
Comment on lines +121 to +159
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excluding this particular, all the other tests are not unnecessary addition in context to the PR. Try to maintain good coding habits and keep the contents of the PR in accordance with the actual context. Everything else can be added little by little in future smaller PRs.


// TestValidateMessage tests the ValidateMessage function.
func TestValidateMessage(t *testing.T) {
testCases := []struct {
name string
message parser.Message
expectedError string
expectedMessage string
}{
{
name: "Valid Message",
message: parser.Message{
Type: "feat",
Scope: "user",
Description: "add a new feature",
},
expectedError: "",
expectedMessage: "valid commit message",
},
{
name: "Invalid Message - Invalid Type",
message: parser.Message{
Type: "Invalid",
Scope: "user",
Description: "add a new feature",
},
expectedError: `invalid commit message type: Invalid`,
expectedMessage: "",
},
{
name: "Invalid Message - Invalid Scope",
message: parser.Message{
Type: "feat",
Scope: "User",
Description: "add a new feature",
},
expectedError: `invalid commit message scope casing, "User" ` +
`should be "user"`,
expectedMessage: "",
},
{
name: "Invalid Message - Invalid Subject",
message: parser.Message{
Type: "feat",
Scope: "user",
Description: "Add a new feature",
},
expectedError: "commit message subject should be lowercased & " +
"not end with a period(.)",
expectedMessage: "",
},
{
name: "Invalid Message - Exceeds Length",
message: parser.Message{
Type: "feat",
Scope: "user",
Description: "add a new feature that makes the application run " +
"faster and more efficiently",
},
expectedError: fmt.Sprintf(
"commit message exceeds 50 characters, current length: %d",
len("Add a new feature that makes the application run "+
"faster and more efficiently"),
),
expectedMessage: "",
},
{
name: "Valid Message - Empty Scope",
message: parser.Message{
Type: "feat",
Scope: "",
Description: "add a new feature",
},
expectedError: "",
expectedMessage: "valid commit message",
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
messageResult, err := ValidateMessage(&tc.message)

if tc.expectedError == "" && err != nil {
t.Errorf("expected no error, got %v", err)
} else if err != nil && err.Error() != tc.expectedError {
t.Errorf("expected error %q, got %q",
tc.expectedError, err.Error())
}

if messageResult != tc.expectedMessage {
t.Errorf("expected message %q, got %q",
tc.expectedMessage, messageResult,
)
}
})
}
}