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] Added unit tests for ErrorReportComponent #12610

Merged
merged 9 commits into from
Nov 29, 2023
Merged

[#12588] Added unit tests for ErrorReportComponent #12610

merged 9 commits into from
Nov 29, 2023

Conversation

u7488099
Copy link
Contributor

@u7488099 u7488099 commented Oct 18, 2023 •
edited
Loading

Fixes [#12588]

Outline of Solution
Add unit tests for ErrorReportComponent

@github-actions
Copy link

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

  • Title must start with the issue number the PR is fixing in square brackets, e.g. [#<issue-number>]
  • 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.

@u7488099 u7488099 changed the title Improve test coverage error report component [#12588] Added unit tests for ErrorReportComponent Oct 18, 2023
@u7488099
Copy link
Contributor Author

Ready for review

cedricongjh
cedricongjh reviewed Oct 18, 2023
View reviewed changes
src/web/app/components/error-report/error-report.component.spec.ts Outdated
Comment on lines 37 to 43
it('should inject ErrorReportService and NgbActiveModal', () => {
const errorReportService = (component as any).errorReportService;
const ngbActiveModal = (component as any).ngbActiveModal;

expect(errorReportService).toBeTruthy();
expect(ngbActiveModal).toBeTruthy();
});
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 remove this test as it's using type assertions (component as any) to access private properties of the component, which is not a recommended practice because it breaks encapsulation, and moreover, we are already testing these 2 services indirectly through the public methods of the classs

cedricongjh
cedricongjh requested changes Oct 18, 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.

a small change, otherwise lgtm!

@cedricongjh cedricongjh mentioned this pull request Oct 18, 2023
Open
@u7488099
Copy link
Contributor Author

The test has been removed

@u7488099
Copy link
Contributor Author

Ready for review

@u7488099
Copy link
Contributor Author

Ready for review

cedricongjh
cedricongjh approved these 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.

LGTM, thank you for your contribution!

@cedricongjh cedricongjh added the s.FinalReview The PR is ready for final review label Oct 21, 2023
@cedricongjh cedricongjh self-assigned this Oct 21, 2023
@github-actions github-actions bot mentioned this pull request Oct 26, 2023
Open
@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...

1 similar comment
@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 9 days). 🐌 😢
Hope someone can get it to move forward again soon...

@weiquu weiquu removed the request for review from jasonqiu212 November 26, 2023 16:38
weiquu
weiquu approved these changes Nov 26, 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 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 26, 2023
@weiquu weiquu merged commit 03569a9 into TEAMMATES:master Nov 29, 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
@u7488099 @weiquu @cedricongjh
[TEAMMATES#12588] Added unit tests for ErrorReportComponent (TEAMMATE…
b4a966f 
…S#12610)

* [TEAMMATES#12588] Added unit tests for ErrorReportComponent

* [TEAMMATES#12588] Added unit tests for ErrorReportComponent

* [TEAMMATES#12588] Added unit tests for ErrorReportComponent

* [TEAMMATES#12588] Added unit tests for ErrorReportComponent

* [TEAMMATES#12588] Fixed Added unit tests for ErrorReportComponent

---------

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
@u7488099 @nusoss-bot @weiquu @cedricongjh @wkurniawan07