Skip to content

Conversation

gunjansingh-msft
Copy link
Member

Description

Please add an informative description that covers that changes made by the pull request and link all relevant issues.

If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.

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.

@github-actions github-actions bot added the Storage Storage Service (Queues, Blobs, Files) label Aug 11, 2025
Copy link
Contributor

github-actions bot commented Aug 11, 2025

API Change Check

APIView identified API level changes in this PR and created the following API reviews

com.azure:azure-storage-file-share

Copy link
Member

@ibrandes ibrandes left a comment

Choose a reason for hiding this comment

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

this looks good! please look into my comments before you merge, but if they do not apply you can ignore them.

Comment on lines +1583 to +1585
String testShareName = generateShareName();
ShareServiceClient serviceClient = primaryFileServiceClient;
ShareClient shareClient = serviceClient.getShareClient(testShareName);
Copy link
Member

Choose a reason for hiding this comment

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

don't we new up a primaryShareClient in the setup method? can we use that instead of creating a new shareClient every time? my question applies to the other tests too

Copy link
Member

Choose a reason for hiding this comment

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

this will be addressed before stg100 is merged in

Comment on lines +1614 to +1616
} finally {
shareClient.delete();
}
Copy link
Member

Choose a reason for hiding this comment

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

why are we deleting the share here? does the automatic test cleanup not detect this share? same with the other tests as well

Copy link
Member Author

Choose a reason for hiding this comment

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

The explicit shareClient.delete(); in the finally block ensures that the test-created share is always deleted, even if an assertion fails or an exception is thrown.

Copy link
Member

Choose a reason for hiding this comment

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

The explicit shareClient.delete(); in the finally block ensures that the test-created share is always deleted, even if an assertion fails or an exception is thrown.

if the assertion fails or an exception is thrown, the test cleanup method kicks in and the share will be deleted. the only time this doesn't apply is if we use a special account, like a premium account. so we should remove these deletes before we merge the 100 branch into main

@gunjansingh-msft gunjansingh-msft merged commit 12ccb88 into feature/storage/stg100base Aug 13, 2025
18 checks passed
@gunjansingh-msft gunjansingh-msft deleted the feature/storage/MakeLeaseConfigurable branch August 13, 2025 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Storage Storage Service (Queues, Blobs, Files)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants