Skip to content

Conversation

@tiffanyfan1015
Copy link
Contributor

Type of changes

  • Feature

Purpose

  • Add Question service test
  • Add Response service test
  • Add Submit service test
  • fix: resolve date type, add continue in response service

Additional Information

  1. Question service
  • Create ->Known Type Success, Unknown Type Failed
  • ListByFormID ->All Known Type return []Answerable, if one type unknown then fail
  1. Response service
  • CreateOrUpdate -> length → not same return error, Exists = true → Update, Exists=false → Create
  • Update -> answer does not exist → create it, answer exists and same → nothing change, answer exists and not same → update
  1. Submit service
  • Submit -> All valid → CreateOrUpdate, some validate wrong → should not call CreateOrUpdate, if some answer.QuestionID not in list → error and not call CreateOrUpdate

Copy link
Member

@YukinaMochizuki YukinaMochizuki left a comment

Choose a reason for hiding this comment

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

I noticed that the Question service adopts table-driven testing, but Response and Submit do not. The current unit tests for Response and Submit suffer from low readability, which makes it difficult to quickly understand the intent of each test case. This will likely cause pain when modifying or adding test cases in the future.

Please at least adopt table-driven testing and include more failure cases to observe any unexpected behaviors, rather than only testing cases with valid formats. When using table-driven tests, please consider separating members such as name, param, setup, and validate, and make sure the name clearly describes the purpose of each test case.

For large mock (fake) structs, please use mockery to generate mocks. This helps maintain readability and future extensibility, while also reducing unnecessary boilerplate code, especially for methods that are not currently in use.

For the table-driven style, consider referring to unit_test.go.

"go.uber.org/zap"
)

type fakeQuerier struct {
Copy link
Member

Choose a reason for hiding this comment

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

For complex mock services, please use mockery.

@tiffanyfan1015 tiffanyfan1015 marked this pull request as draft October 12, 2025 14:55
@tiffanyfan1015 tiffanyfan1015 self-assigned this Oct 12, 2025
@tiffanyfan1015 tiffanyfan1015 force-pushed the feat/CORE-75-form-unit-test branch from 5e51807 to ab26aa7 Compare October 14, 2025 17:36
@tiffanyfan1015 tiffanyfan1015 marked this pull request as ready for review October 14, 2025 18:26
@tiffanyfan1015 tiffanyfan1015 requested a review from dytsou October 14, 2025 18:29
Copy link
Member

@dytsou dytsou left a comment

Choose a reason for hiding this comment

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

Some style issues have to be fix. Since Golang has a strict convention on naming, the abbreviated variables should be straighforward, or we often uses full names.

There are some similar cases in other lines that I have not listed below would also have to be fixed.

@tiffanyfan1015 tiffanyfan1015 requested a review from dytsou October 18, 2025 10:40
@tiffanyfan1015 tiffanyfan1015 requested a review from dytsou October 20, 2025 13:11
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