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

test(calling): voicemail module test cases added #3107

Conversation

meta-dipanshu-sharma
Copy link
Collaborator

@meta-dipanshu-sharma meta-dipanshu-sharma commented Sep 24, 2023

COMPLETES #< SPARK-422971 >

This pull request addresses

Code coverage for voicemail module files were less. Need to write UT to cover the uncovered code.

by making the following changes

Added UTs for following files:

  1. BroadworksBackendConnector.test.ts
  2. UcmBackendConnector.test.ts
  3. Voicemail.test.ts

Some code refactor in below files:

  1. BroadworksBackendConnector.ts
  2. UcmBackendConnector.ts
  3. Utils.ts

CODE COVERAGE REPORT:
PREVIOUSLY
Screenshot 2023-09-24 at 11 40 01 PM

CURRENT:
Screenshot 2023-09-26 at 2 21 30 PM

Change Type

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Tooling change
  • Internal code refactor

The following scenarios where tested

< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >

I certified that

  • I have read and followed contributing guidelines

  • I discussed changes with code owners prior to submitting this pull request

  • I have not skipped any automated checks

  • All existing and new tests passed

  • I have updated the documentation accordingly


Make sure to have followed the contributing guidelines before submitting.

@meta-dipanshu-sharma meta-dipanshu-sharma requested a review from a team as a code owner September 24, 2023 18:11
@meta-dipanshu-sharma meta-dipanshu-sharma added the validated If the pull request is validated for automation. label Sep 24, 2023
@meta-dipanshu-sharma meta-dipanshu-sharma force-pushed the voicemail-folder-unit-test branch 2 times, most recently from 9659ce3 to 28aef70 Compare September 25, 2023 11:49
Copy link
Contributor

@karthikraji2020 karthikraji2020 left a comment

Choose a reason for hiding this comment

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

Left few comments. please check.

const mediaType = respHeaders?.mediatype as string;
const mediaContent = contentInfo as string;
const responseDetails = {
statusCode: response.statusCode as number,
statusCode: statusCode as number,
Copy link
Contributor

Choose a reason for hiding this comment

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

as number is known to be problematic and we are changing it whenever we encounter it during new changes. Applicable across all files in the module. Please add this change.

Suggested change
statusCode: statusCode as number,
statusCode: Number(statusCode),

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@@ -465,7 +465,9 @@ export class BroadworksBackendConnector implements IBroadworksCallBackendConnect
return responseDetails;
} catch (err: unknown) {
/* Catch the exception error code from try block, return the error object to user */
const errorInfo = err as WebexRequestPayload;
const errorInfo = {
statusCode: err instanceof Error ? Number(err.message) : '',
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we not set '' as default statusCode. Put 422 or equivalent code. Applicable across the changes.

Suggested change
statusCode: err instanceof Error ? Number(err.message) : '',
statusCode: err instanceof Error ? Number(err.message) : 422,

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We're setting default statusCode as 422 in serviceErrorCodeHandler in the default switch statement when it doesn't matches with any of the status code. If we want to set it default here then we have to update it at multiple places wherever ' ' is passsed. Do you still suggest changing this at multiple places or current implementation is acceptable?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thats fine . Ignore this if it was like this

Copy link
Contributor

@BhargavSatya BhargavSatya left a comment

Choose a reason for hiding this comment

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

Minor comments.
Can we try to increase branch coverage to minimum of 85% as well?

Copy link
Contributor

@Shreyas281299 Shreyas281299 left a comment

Choose a reason for hiding this comment

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

Minor comments.

@meta-dipanshu-sharma
Copy link
Collaborator Author

Minor comments. Can we try to increase branch coverage to minimum of 85% as well?

@BhargavSatya I have made BroadworksBackendConnector file's branch coverage to >85%. For UcmBackendConnector file, we were not able to cover some part of code and those branches are part of those lines only so could not cover them as well.

@meta-dipanshu-sharma meta-dipanshu-sharma merged commit dceac49 into webex:next Sep 26, 2023
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
validated If the pull request is validated for automation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants