Skip to content

Reduce Test Times from Slow Tests#31954

Merged
alzimmermsft merged 1 commit intoAzure:mainfrom
alzimmermsft:AzTest_ReduceCiTimes
Nov 4, 2022
Merged

Reduce Test Times from Slow Tests#31954
alzimmermsft merged 1 commit intoAzure:mainfrom
alzimmermsft:AzTest_ReduceCiTimes

Conversation

@alzimmermsft
Copy link
Copy Markdown
Member

Description

  • Updated Confidential Ledger tests to use a 10ms poll delay in playback instead of the default of 30 seconds.
  • Mark Resource Manager test to only run in live testing as it takes 19 minutes.

All SDK Contribution checklist:

  • The pull request does not introduce [breaking changes]
  • CHANGELOG is updated for new features, bug fixes or other significant changes.
  • I have read the contribution guidelines.

General Guidelines and Best Practices

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

@alzimmermsft alzimmermsft self-assigned this Nov 4, 2022
@azure-sdk
Copy link
Copy Markdown
Collaborator

API change check

API changes are not detected in this pull request.

@alzimmermsft alzimmermsft marked this pull request as ready for review November 4, 2022 18:11
@alzimmermsft alzimmermsft enabled auto-merge (squash) November 4, 2022 18:13
@joshfree
Copy link
Copy Markdown
Member

joshfree commented Nov 4, 2022

Thanks @alzimmermsft

@alzimmermsft
Copy link
Copy Markdown
Member Author

/check-enforcer override

@alzimmermsft alzimmermsft merged commit 1034e2c into Azure:main Nov 4, 2022
@alzimmermsft
Copy link
Copy Markdown
Member Author

Overriding check enforcer as all tests passed and no linting changes were made

@alzimmermsft alzimmermsft deleted the AzTest_ReduceCiTimes branch November 4, 2022 18:27
} else if (getTestMode() == TestMode.PLAYBACK) {
ledgerManager = ConfidentialLedgerManager
.configure()
.withDefaultPollInterval(Duration.ofMillis(10))
Copy link
Copy Markdown
Member

@weidongxu-microsoft weidongxu-microsoft Nov 7, 2022

Choose a reason for hiding this comment

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

Thanks.

... one more reason to avoid RECORD test. (these mgmt lib generally only do live tests, ran before release)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@alzimmermsft It occured to me that DPG lib might have LRO tests as well. How does data-plane handle it?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Many of the data-plane tests end up having a method for setting the poll interval based on playback or live testing but there are methods available on TestBase which handle setting this: https://github.com/Azure/azure-sdk-for-java/blob/main/sdk/core/azure-core-test/src/main/java/com/azure/core/test/TestBase.java#L286

Ideally, those would be used more often.

Copy link
Copy Markdown
Member

@weidongxu-microsoft weidongxu-microsoft Nov 8, 2022

Choose a reason for hiding this comment

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

I see, so "setPlaybackSyncPollerPollInterval" should be called after getting the SyncPoller and before calling it to get final result, by dev in test code. Is it correct?

If so, I will update to the guide for DPG on test section.

PS: But from impl, it seems "RetryAfter" from response still take precedence over "pollInterval"? https://github.com/Azure/azure-sdk-for-java/blob/main/sdk/core/azure-core/src/main/java/com/azure/core/util/polling/PollingUtil.java#L101-L110

Comment on lines +198 to 201
@DoNotRecord(skipInPlayback = true)
public void testCloneVirtualMachineToNewRegion() {
Assertions.assertTrue(CloneVirtualMachineToNewRegion.runSample(azureResourceManager));
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@XiaofeiCao Please check why PLAYBACK is slow. Should use ResourceManagerUtils.getDelayDuration for any duration in code.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@XiaofeiCao Also please check if JUnit can have a global setting for timeout for each test case (e.g. set it to 5 sec per test, enabled only at PLAYBACK).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Got it, The issue is that I did not use the ResourceManagerUtils.getDelayDuration in waitForCopyStartCompletion.

Sorry for that.. Will look into it.

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.

6 participants