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

Fix #7921 and added test cases #892

Merged
merged 13 commits into from
Nov 10, 2023
Merged

Fix #7921 and added test cases #892

merged 13 commits into from
Nov 10, 2023

Conversation

ayeminag
Copy link
Contributor

@ayeminag ayeminag commented Oct 2, 2023

NOTES

In order to run internal/e2e-realtime-api/voiceSpeechCollect.test.ts before merging the backend changed

Backend Todos

  • setup devbox with FS changes
  • setup steering to devbox
  • create a sip domain app on staging and handle the call on domain app with a relay context relaye2e
  • add routing to devbox in kamailio for the domain app you set up in previous step

SDK Todos

  • in internal/e2e-realtime-api/src/voiceSpeechCollect.test.ts, replace dev-min.dapp.swire.io sip domain with the sip domain you created in above step.
  • copy env file into internal/e2e-realtime-api/.env.test and setup
  • run npm dev:rt-only

References

Description

  • Added state param to CallingCallCollectEventParams
  • Made sure voiceCallCollectWorker doesn't clean up CallCollect instance and emit ended/failed event if the state is "collecting"
  • Resolve CallCollect.ended() promise only when state is NOT "collecting" AND final is either undefined/true AND result.type is one of ENDED_STATES.
  • Added more test cases for Call.collect() in @sw-internal/e2e-realtime-api

@changeset-bot
Copy link

changeset-bot bot commented Oct 2, 2023

🦋 Changeset detected

Latest commit: f2c8332

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@sw-internal/e2e-realtime-api Patch
@signalwire/realtime-api Patch
@signalwire/core Patch
@signalwire/js Patch
@signalwire/web-api Patch
@signalwire/webrtc Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@ayeminag ayeminag changed the title TEST: added test to prove cloud-product#7921 (Don't Merge) TEST: added test to prove cloud-product#7921 Oct 31, 2023
Aye Min Aung added 2 commits October 31, 2023 12:41
…e and emit ended/failed event if the state is "collecting" and resolve CallCollect.ended() promise only when state is not "collecting" AND final is either undefined/true AND result.type is one od ENDED_STATES.
@ayeminag ayeminag requested a review from giavac October 31, 2023 06:00
@ayeminag ayeminag changed the title TEST: added test to prove cloud-product#7921 Fix #7921 and added test cases Oct 31, 2023
@ayeminag ayeminag requested a review from iAmmar7 October 31, 2023 06:08
@ayeminag
Copy link
Contributor Author

ayeminag commented Nov 3, 2023

@giavac I added the steps we need to take to run test before backend changes are on staging.

giavac
giavac previously requested changes Nov 3, 2023
Copy link
Collaborator

@giavac giavac left a comment

Choose a reason for hiding this comment

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

Bookmark to ensure the generic SIP Domains are restored before merging.

@ayeminag
Copy link
Contributor Author

ayeminag commented Nov 6, 2023

Update: No special steps needed to run the new test cases anymore to test on staging.

@giavac
Copy link
Collaborator

giavac commented Nov 6, 2023

(I'll approve once the other open conversations will be resolved)

@ayeminag
Copy link
Contributor Author

ayeminag commented Nov 10, 2023

I updated the tests to make sure to fail on prod, but it seems like we deployed the collect changes to production already.
But if the state is missing in continuous: true case, SDK will just resolve the collect.ended() and cleanup CallCollect instance on first collect event with final: true (in case of partialResults: true) OR on first speech type collect event (in case partialResults: false | undefined). And when new events come in after that sdk will throws Error('Missing the Instance'), sdk is already catching that error and logging it to stderr. So, I added tests to make sure Missing the Instance error wasn't logged to stderr, if the event is missing state in continuous: true case tests will fail.

I updated the audio file I am playing in the test to include long enough pause, so FS will deliver more than one collect events (at least one event with final: true in partialResults: true, continuous: true case), and adjusted silenceTimeout and speechTimeout to accommodate it.

Since, changes are on prod, to test the failure. I disabled the state !== 'collecting' checks in sdk first (because not checking for state on sdk side is behaviorally same as state missing from FS side events), and ran the test, tests failed because it's getting Missing the Instance error just like it should. See the logs here.

I also notice speech detect can return inconsistant data for example 1 2 3 4 5 instead of one two three four five I updated assert for all possible cases, so they will pass as long as it's one of the possible expected texts.

/cc @iAmmar7

Copy link
Collaborator

@iAmmar7 iAmmar7 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @ayeminag 🚀

@ayeminag ayeminag merged commit d564c37 into main Nov 10, 2023
9 of 10 checks passed
@ayeminag ayeminag deleted the min/proof-7921 branch November 10, 2023 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants