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

[VBLOCKS-1715] feat: outgoing call test #64

Merged
merged 10 commits into from
Jun 2, 2023
Merged

Conversation

kpchoy
Copy link
Contributor

@kpchoy kpchoy commented May 31, 2023

Submission Checklist

  • The CHANGELOG.md reflects any feature, bug fixes, or known issues made in the source code
  • New unit tests and integration tests have beed added
  • Code coverage is more or equal to the previous coverage
  • A visual inspection of the Files changed tab was made prior to submitting the pull request ensuring the style guide was followed
  • CI pipeline is green
  • Included screenshot if necessary

Description

VBLOCKS-1715

Breakdown

  • outgoing call UI test
  • success case (android only)
  • disconnect cases

Validation

  • [Bulleted summary of validation steps including local and CI pipeline]
  • [eg. Added new test]
  • [eg. Manually tested with both app and server]

Additional Notes

[Any additional comments, notes, or information relevant to reviewers.]

Contributing to Twilio

All third-party contributors acknowledge that any contributions they provide will be made under the same open-source license that the open-source project is provided under.

  • I acknowledge that all my contributions will be made under the project's license.

@kpchoy kpchoy changed the title feat: outgoing call test [VBLOCKS-1715] feat: outgoing call test May 31, 2023
@kpchoy kpchoy requested a review from charliesantos June 1, 2023 00:03
Copy link
Contributor

@charliesantos charliesantos left a comment

Choose a reason for hiding this comment

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

Left some comments. We should also add tests that covers the following:

  • check if call is disconnected after clicking end button
  • check if call is disconnected after callee disconnects the call or times out
    Since this is a timebox task, this can be part of next sprint (new ticket) if you run out of time this sprint.

CHANGELOG.md Outdated Show resolved Hide resolved
app/e2e/outgoingcall.test.js Outdated Show resolved Hide resolved
await element(by.id('call_button')).tap();
await waitFor(element(by.id('active_call'))).toBeVisible();
await waitFor(element(by.id('call_status'))).toHaveText('ringing');
await waitFor(element(by.id('call_status'))).toHaveText('disconnected');
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also check that the call gets disconnected right away? It should get disconnected in less than 3s, is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it can be right away, since the UI still needs time to switch Views. That is why I used a waitFor, since without it the test fails. Where does the 3s come from?

Copy link
Contributor

Choose a reason for hiding this comment

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

By "right away" I mean, it should not feel like it's stuck. 3s is just an estimate. What we want is, we need to fail this test if it exceeds a certain threshold. For example, if it does not disconnect after 20s, there must be something wrong. 20s is too extreme, so I estimated around 3s. How long do you think we should wait before we consider this as a fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I get what you mean now. Okay will add another testcase. 3s sounds like a good time to me 👍

await waitFor(element(by.id('active_call'))).not.toBeVisible();
});

it('should disconnect if invalid Client-ID', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, we should check that the call has been disconnected right away.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Disconnect still doesn't happen right away. We still have the user see texts ringinging -> disconnect -> back to dial screen.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same as my response here #64 (comment)

Copy link
Contributor

@charliesantos charliesantos left a comment

Choose a reason for hiding this comment

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

Left some comments. We should also add tests that covers the following:

  • check if call is disconnected after clicking end button
  • check if call is disconnected after callee disconnects the call or times out
    Since this is a timebox task, this can be part of next sprint (new ticket) if you run out of time this sprint.

@kpchoy
Copy link
Contributor Author

kpchoy commented Jun 1, 2023

  • check if call is disconnected after clicking end button

I tried to do test case disconnect after clicking end button , but the test would always fail. I believe this a detox issue, and the issue is still open wix/Detox#1185

No views in hierarchy found matching: (with tag value: is "<Test ID>" and view has effective visibility=VISIBLE

@kpchoy
Copy link
Contributor Author

kpchoy commented Jun 1, 2023

  • check if call is disconnected after callee disconnects the call or times out

Can setup another number and use Twilio verb to end

@kpchoy kpchoy requested a review from charliesantos June 1, 2023 18:58
@@ -0,0 +1,82 @@
import { by, element, expect, waitFor, device } from 'detox';

describe('Outgoing Call', () => {
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 continue the discussion here regarding the following test case, so we can separate the thread

  • check if call is disconnected after clicking end button

I tried to do test case disconnect after clicking end button , but the test would always fail. I believe this a detox issue, and the issue is still open wix/Detox#1185

Clicking the end button behaves the same as when the call is disconnected correct? Can we assert that the active call screen is not visible anymore and that it will transition to the next screen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had tried various methods earlier in the week, so I didn't think this was possible. But I tried a few more methods yesterday and today, now I have one that works. I have now included a test for your test case.

@@ -0,0 +1,82 @@
import { by, element, expect, waitFor, device } from 'detox';

describe('Outgoing Call', () => {
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 continue the discussion here regarding the following test case, so we can separate the thread

  • check if call is disconnected after callee disconnects the call or times out

Can setup another number and use Twilio verb to end

I don't think we need to. We can just wait until the current call times out? It's only 10s long right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes call is only 10s. Ok will add a 12s timer to make sure call is no longer in progress

@kpchoy kpchoy requested a review from charliesantos June 2, 2023 18:51
Copy link
Contributor

@charliesantos charliesantos left a comment

Choose a reason for hiding this comment

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

Nice! I'm glad we're able to cover all the cases we talked about. +1 for me.
I also left a couple of questions. You can merge the PR after answering.

await element(by.id('dialpad_button_5')).tap();
await element(by.id('dialpad_button_4')).tap();
await element(by.id('dialpad_button_1')).tap();
await device.disableSynchronization();
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Detox tries to optimize by "synchronizing" https://wix.github.io/Detox/docs/troubleshooting/synchronization/ . But I think there is some kind of bug triggered by their optimization when trying to click the "end call" button. When in the normal "synchronizing" mode detox cannot find the "end call" button, even though I see it as detox steps through the test case. I found a stackoverflow where someone recommended disabling this "testing opimization" https://stackoverflow.com/questions/59412749/detox-detect-that-element-was-displayed

.toExist()
.withTimeout(3000);
await element(by.id('end_call_button')).tap();
await device.enableSynchronization();
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above ^

@kpchoy kpchoy merged commit 21543f3 into main Jun 2, 2023
@kpchoy kpchoy deleted the feat-call-ui-testing branch June 2, 2023 19:07
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.

2 participants