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

[#11826] Add unit tests for SubmitFeedbackResponsesAction #12234

Conversation

undermyumbrella1
Copy link
Contributor

Fixes #11826
Questions:

  1. SubmitFeedbackResponsesAction has dependencies on several other classses. Should these dependencies by stubbed? From similar unit tests of other components, dependencies are not subbed.
  2. In SubmitFeedbackResponseAction:execute(), if FeedbackResponseDetails is null, an AssertionError is thrown. May I clarify that this behaviour does not need to be tested? (as it is an expected behaviour)
  3. Not sure if I understood the type of questions that a Feedback Response can be issued for, is correctly used.

Copy link
Contributor

@zhaojj2209 zhaojj2209 left a comment

Choose a reason for hiding this comment

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

Thank you for your PR! To answer your questions:

  1. No need to stub them, current approach is alright. However, do note that as part of our ongoing database migration project, our test code will undergo an overhaul as well, so dependencies may be mocked in the future. We will aim to have your PR resolved before then so that you won't have to make too many changes to your PR.
  2. Yes, no need to test this.
  3. Looks alright to me.

Some slight adjustments on my end. Personally I would also say that some private helper functions are not very necessary, but it's not a big issue.

@zhaojj2209 zhaojj2209 added the s.ToReview The PR is waiting for review(s) label Mar 22, 2023
Copy link
Contributor

@zhaojj2209 zhaojj2209 left a comment

Choose a reason for hiding this comment

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

LGTM!

No worries regarding what I mentioned previously about the abstraction of private helper functions. Whether a piece of code is over-abstracted is rather subjective, which is why I did not request for further changes.

For your own learning, though, the SWE @ Google chapter on unit testing explains DRY in testing pretty well, so it's worth a read!

Copy link
Member

@samuelfangjw samuelfangjw 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 on this!

We will merge this into main, but do note that we are currently working on an overhaul of the backend, including the tests. These test classes will be removed or migrated in the month or so.

SubmitFeedbackResponsesAction has dependencies on several other classses. Should these dependencies by stubbed? From similar unit tests of other components, dependencies are not subbed.

Unit Tests here is really a misnomer. These really should be called integration tests instead. We are looking to change this in the near future with a more traditional mix of unit and integration tests. Feel free to have a look if you are interested, though note that the tests on this branch require a few more rounds of polishing before I would consider them ready to be merged.

@samuelfangjw samuelfangjw added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.ToReview The PR is waiting for review(s) labels Mar 22, 2023
@samuelfangjw samuelfangjw added this to the V8.26.0 milestone Mar 22, 2023
@samuelfangjw samuelfangjw merged commit 84608ec into TEAMMATES:master Mar 23, 2023
@zhaojj2209 zhaojj2209 added the c.Task Other non-user-facing works, e.g. refactoring, adding tests label Apr 2, 2023
@undermyumbrella1 undermyumbrella1 deleted the 11826/submitfeedbackresponsesaction-test branch May 4, 2023 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c.Task Other non-user-facing works, e.g. refactoring, adding tests s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add unit tests for SubmitFeedbackResponsesAction
3 participants