Skip to content

Conversation

@0rland0Wats0n
Copy link
Contributor

This PR refactors phone number tests and runs them in a matrix. This is the same approach used by ai-form-recognizer here

export function matrix<T extends ReadonlyArray<readonly unknown[]>>(

Big up ai-form-recognizer 🕺🏾

@willmtemple @bterlson @xirzec this matrix method is a cool, and powerful, tool. Would be cool if it was more visibly accessible.

@0rland0Wats0n 0rland0Wats0n requested review from a team and ankitarorabit as code owners April 13, 2021 22:22
@ghost ghost added the Communication label Apr 13, 2021
Copy link
Member

@beltr0n beltr0n left a comment

Choose a reason for hiding this comment

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

code looks good, just a question about how timeout values are determined

const { phoneNumber } = await client.getPurchasedPhoneNumber(purchasedPhoneNumber);

assert.strictEqual(purchasedPhoneNumber, phoneNumber);
}).timeout(7000);
Copy link
Member

Choose a reason for hiding this comment

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

How are these lengths determined btw?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Default timeout is 2 seconds. If a test fails because of time out I increase to 5 seconds and rerun. If it still fails I increase by 5 seconds and rerun until I get no failures. I then reduce timeout to 1 second above the slowest run. So for this test case, slowest run was ~6 seconds.

For LROs, I run them without a time limit first, timeout(0), then set timeout based on how long they take.

@0rland0Wats0n
Copy link
Contributor Author

/azp run js - communication-phone-numbers - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

* });
* ```
*/
export function matrix<T extends ReadonlyArray<readonly unknown[]>>(
Copy link
Member

Choose a reason for hiding this comment

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

@deyaaeldeen @willmtemple
We're stealing this nifty code for a couple of our services, is there a common place for the entire SDK where it could be shared?

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps it could live here? https://github.com/Azure/azure-sdk-for-js/blob/master/sdk/test-utils/multi-version/ ?

Or we could make a new test-utils package

@0rland0Wats0n
Copy link
Contributor Author

/azp run js - communication-phone-numbers - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@0rland0Wats0n
Copy link
Contributor Author

I had to remove buildCapabilityUpdate 😭. Making a new update request per test run causes assert.deepEqual(phoneNumber.capabilities, update) to fail in the pipeline. This is because pipeline environments are run in parallel and each trigger an update to the same phone number. I simulated locally by running the test case in 2 separate consoles.

@0rland0Wats0n
Copy link
Contributor Author

/azp run js - communication-phone-numbers - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@0rland0Wats0n
Copy link
Contributor Author

/azp run js - communication-phone-numbers - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@0rland0Wats0n 0rland0Wats0n merged commit 817d7c2 into Azure:master Apr 14, 2021
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Jun 21, 2021
Webpubsub 2021-06-01-preview (Azure#14868)

* init api version 2021-06-01-preview for from 2021-04-01-preview

* update example

* update readme

* update swagger file

Co-authored-by: Binjie Qian <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants