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

feat: breaking out voice calls into a testable function #2768

Conversation

AlaricWhitney
Copy link
Collaborator

  • Identified the issue which this PR solves.
  • Read the CONTRIBUTING document.
  • Code builds clean without any errors or warnings.
  • Added appropriate tests for any new functionality.
  • All new and existing tests passed.
  • Added comments in the code, where necessary.
  • Ran make check to catch common errors. Fixed any that came up.

Description:
Broke out the building of a phone call from a large monolithic function into its own, smaller, and testable function.

Which issue(s) this PR fixes:
Part of addressing #2576

Out of Scope:
changes to the voice xml building functionality.

Screenshots:
NA

Describe any introduced user-facing changes:
NA

Describe any introduced API changes:
NA

Additional Info:
NA

notification/twilio/voice.go Outdated Show resolved Hide resolved
notification/twilio/voice.go Outdated Show resolved Hide resolved
@AlaricWhitney AlaricWhitney requested review from mastercactapus and cmarquis and removed request for cmarquis and mastercactapus February 8, 2023 23:00
cmarquis
cmarquis previously approved these changes Feb 27, 2023
@mastercactapus
Copy link
Member

A table test isn't a good fit for testing BuildMessage; each case is pretty specific to the message type, and the context between what is being tested and the actual assertion is separated by over 100 lines.

Better would be to either break each message type into its unit test (e.g., TestBuildMessage_Alert TestBuildMessage_AlertStatus) or at least to flatten the test as I suggested here: #2768 (comment) To keep the assertions near the inputs.

Table testing is helpful for simple cases, like strings, or lots of variation when all inputs are very similar. Each message type is different in this case, and it doesn't help to bundle them together because, for each set, the input and output for each message type are drastically different.

@AlaricWhitney AlaricWhitney dismissed mastercactapus’s stale review May 1, 2023 14:53

took care of it by taking it it out of the table. dismissing since the item has now been addressed so we can move forward

@AlaricWhitney AlaricWhitney merged commit 19dace4 into target:master May 1, 2023
@AlaricWhitney AlaricWhitney deleted the feat--breaking-out-voice-calls-into-a-testable-function branch May 1, 2023 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants