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

Drop tables in Dispose() for ConversionTests #1251

Merged
merged 1 commit into from
Sep 8, 2021
Merged

Drop tables in Dispose() for ConversionTests #1251

merged 1 commit into from
Sep 8, 2021

Conversation

johnnypham
Copy link
Contributor

Attempt to fix #1230

The queries to drop the tables are moved from the end of each test method to Dispose(), to drop the tables before dropping the CEKs.

@johnnypham johnnypham added this to the 4.0.0-preview2 milestone Sep 8, 2021
Copy link
Member

@DavoudEshtehari DavoudEshtehari left a comment

Choose a reason for hiding this comment

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

It seems reasonable, and I couldn't see any issue with it. Just be cautious! We had a known intermittent issue with AE tests, which were related to the Dispose functionality in the fixture on facing any exception.

cc @JRahnama

@JRahnama
Copy link
Member

JRahnama commented Sep 8, 2021

It seems reasonable, and I couldn't see any issue with it. Just be cautious! We had a known intermittent issue with AE tests, which were related to the Dispose functionality in the fixture on facing any exception.

The issue is a known issue, as far as I could research, in XUnit when using IClassFixture. The dispose is not called till all the tests related to the fixture is ran to completion. If the tests are stopped or aborted at the middle no Dispose is called to clean up after.

However, As Davoud mentioned we wont know the result till we see the tests in action with this change as the issue was happening intermittently. Looks good to as well.

@JRahnama JRahnama merged commit aea4bbe into dotnet:main Sep 8, 2021
@johnnypham johnnypham deleted the issue1230 branch September 9, 2021 20:05
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.

Test AlwaysEncrypted.ConversionTests.ConversionSmallerToLarger... fails
3 participants