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

Attemped unit tests + refactoring code #53

Closed
wants to merge 3 commits into from
Closed

Attemped unit tests + refactoring code #53

wants to merge 3 commits into from

Conversation

abyan-n
Copy link
Collaborator

@abyan-n abyan-n commented Oct 17, 2024

Description

No Ticket

Please include a summary of the changes and the related issue. Please also
include relevant motivation, context, and images!

Adding unit tests for friendship handler routes -- only create friendship as of now

Refactored createFriendship endpoint and the route definiton

How Has This Been Tested?

Unit tests

Checklist

  • [ X] I have performed a self-review of my code
  • [ X] I have reached out to another developer to review my code
  • [ X] I have commented my code, particularly in hard-to-understand areas
  • [ X] New and existing unit tests pass locally with my changes

}

// Implement the CreatePreferences method for the MockStore
func (m *MockStore) CreatePreferences(ctx context.Context, preferences models.Preferences) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This mock looks right, super clean

return nil
}

// Implement the UpdateProfilePreferences method for the MockStore
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great work abyan, looks awesome

Copy link
Member

@garrettladley garrettladley left a comment

Choose a reason for hiding this comment

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

great work here @abyan-n !

return c.Status(fiber.StatusCreated).JSON(req)
// Call CreateFriendship method to interact with db
if err := s.store.CreateFriendship(c.Context(), req); err != nil {
return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{
Copy link
Member

Choose a reason for hiding this comment

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

with the central error handler, can just bubble up an errors.New("failed to create friendship")

var req models.Friendship
// Parse the request body and check for errors
if err := c.BodyParser(&req); err != nil {
return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{
Copy link
Member

Choose a reason for hiding this comment

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

would recommend using errs.InvalidJSON(err). would refactor that func to be

func InvalidJSON(err error) APIError {
	return NewAPIError(http.StatusBadRequest, err)
}

so we can drill down the actual error to the client. more ergonomic since we can tell them they forgot to close with a } instead of a blanket Cannot parse JSON

}

// On success, return 201 and req body
return c.Status(fiber.StatusCreated).JSON(req)
Copy link
Member

Choose a reason for hiding this comment

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

good stuff

)

// Mock Service structure
type MockStore struct {
Copy link
Member

Choose a reason for hiding this comment

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

would prob want to convert this to and interface then move it to backend/internal/handlers/friendship/friendship.go. then have your actual and your mock impl the interface.

@CamPlume1 CamPlume1 closed this Dec 5, 2024
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.

4 participants