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.

Sign up for GitHub

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

Jump to bottom

[#12588] Add unit test for every method in CommentRowComponent #12612

Merged
merged 14 commits into from
Nov 30, 2023
Merged

[#12588] Add unit test for every method in CommentRowComponent #12612

merged 14 commits into from
Nov 30, 2023

Conversation

ThomasGreen123
Copy link
Contributor

@ThomasGreen123 ThomasGreen123 commented Oct 19, 2023 •
edited by weiquu
Loading

Part of #12588

Outline of Solution
Add unit test for every method in CommentRowComponent

@github-actions
Copy link

Hi @ThomasGreen123, thank you for your interest in contributing to TEAMMATES!
However, your PR does not appear to follow our contribution guidelines:

  • Description must reference the issue number the PR is fixing, e.g. Fixes #<issue-number> (or Part of #<issue-number> if the PR does not address the issue fully)

Please address the above before we proceed to review your PR.

@ThomasGreen123
Copy link
Contributor Author

Ready for review

cedricongjh
cedricongjh requested changes Oct 19, 2023
View reviewed changes
Copy link
Contributor

@cedricongjh cedricongjh 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!

do take a look at the comments and make the changes accordingly

@ThomasGreen123
Copy link
Contributor Author

Thank you for giving me these instructions! I have made changes accordingly. Does this meet requirements now? Feel free to tell me if this requires further improvement.

cedricongjh
cedricongjh requested changes Oct 21, 2023
View reviewed changes
Copy link
Contributor

@cedricongjh cedricongjh left a comment

Choose a reason for hiding this comment

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

hey @ThomasGreen123, you're almost there with this PR, let's expect that the allowAllApplicableTypesToSee and getNewVisibilityStateMachine are called with the correct parameters, similar to the example i provided earlier with applyVisibilitySettings

@cedricongjh cedricongjh mentioned this pull request Oct 22, 2023
Open
@cedricongjh cedricongjh self-assigned this Oct 22, 2023
@cedricongjh cedricongjh added the s.ToReview The PR is waiting for review(s) label Oct 22, 2023
@github-actions github-actions bot mentioned this pull request Oct 26, 2023
Open
@ThomasGreen123
Copy link
Contributor Author

Yeah, I have made changes accordingly. Ready for review.

cedricongjh
cedricongjh requested changes Oct 28, 2023
View reviewed changes
Copy link
Contributor

@cedricongjh cedricongjh left a comment •
edited
Loading

Choose a reason for hiding this comment

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

apologises for the late review, just a few more changes and we're good to go!

src/web/app/components/comment-box/comment-row/comment-row.component.spec.ts Outdated Show resolved Hide resolved
src/web/app/components/comment-box/comment-row/comment-row.component.spec.ts Outdated Show resolved Hide resolved
src/web/app/components/comment-box/comment-row/comment-row.component.spec.ts Outdated

expect(spyVisibilityStateMachine.allowAllApplicableTypesToSee).toHaveBeenCalledWith();

expect(spyCommentService.getNewVisibilityStateMachine).toHaveBeenCalledWith([]);
Copy link
Contributor

Choose a reason for hiding this comment

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

let's set questionShowResponsesTo in the component, and check that getNewVisibilityStateMachine is called with the appropriate params

@ThomasGreen123
[TEAMMATES#12588] Fix the error
2fb3060
@ThomasGreen123
Merge remote-tracking branch 'origin/master'
4fb1d5d 
# Conflicts:
#	src/web/app/components/comment-box/comment-row/comment-row.component.spec.ts
@ThomasGreen123
[TEAMMATES#12588] Fix the error
101af47
@ThomasGreen123
Copy link
Contributor Author

Thank you for your advice. I've made relevant changes. Ready for review.

cedricongjh
cedricongjh approved these changes Oct 30, 2023
View reviewed changes
Copy link
Contributor

@cedricongjh cedricongjh left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for the work on this!

@cedricongjh cedricongjh added s.FinalReview The PR is ready for final review and removed s.ToReview The PR is waiting for review(s) labels Oct 30, 2023
@nusoss-bot
Copy link

Folks, This PR seems to be stalling (no activities for the past 7 days). 🐌 😢
Hope someone can get it to move forward again soon...

@nusoss-bot
Copy link

Folks, This PR seems to be stalling (no activities for the past 8 days). 🐌 😢
Hope someone can get it to move forward again soon...

@nusoss-bot
Copy link

Folks, This PR seems to be stalling (no activities for the past 11 days). 🐌 😢
Hope someone can get it to move forward again soon...

weiquu
weiquu approved these changes Nov 30, 2023
View reviewed changes
Copy link
Contributor

@weiquu weiquu left a comment

Choose a reason for hiding this comment

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

LGTM

@weiquu weiquu removed the request for review from jasonqiu212 November 30, 2023 07:01
@weiquu weiquu added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.FinalReview The PR is ready for final review labels Nov 30, 2023
@weiquu weiquu merged commit 6d0084e into TEAMMATES:master Nov 30, 2023
9 checks passed
@wkurniawan07 wkurniawan07 added the c.Task Other non-user-facing works, e.g. refactoring, adding tests label Jan 21, 2024
@wkurniawan07 wkurniawan07 added this to the V8.30.0 milestone Jan 21, 2024
cedricongjh pushed a commit to cedricongjh/teammates that referenced this pull request Feb 20, 2024
@ThomasGreen123 @weiquu @cedricongjh
[TEAMMATES#12588] Add unit test for every method in CommentRowCompone…
45fff0a 
…nt (TEAMMATES#12612)

* Add unit test for every method in comment-row.component.spec.ts

* [TEAMMATES#12588] Add one unit test in comment-row.component.spec.ts

* [TEAMMATES#12588] Update on unit test in comment-row.component.spec.ts

* [TEAMMATES#12588] Update on small typo in comment-row.component.spec.ts

* [TEAMMATES#12588] Update on small change in comment-row.component.spec.ts

* [TEAMMATES#12588] Add unit test for every method in recipient-type-name.pipe.spec.ts

* [TEAMMATES#12588] fix some typo

* [TEAMMATES#12588] fix some typo

* [TEAMMATES#12588] Add some unit test in recipient-type-name.pipe.spec.ts

* [TEAMMATES#12588] Fix the error

* [TEAMMATES#12588] Fix the error

---------

Co-authored-by: Wei Qing <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cedricongjh cedricongjh cedricongjh approved these changes

@weiquu weiquu weiquu approved these changes

Assignees

@cedricongjh cedricongjh

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
Milestone
V8.30.0
Development

Successfully merging this pull request may close these issues.
5 participants
@ThomasGreen123 @nusoss-bot @weiquu @cedricongjh @wkurniawan07