Skip to content

Remove deprecated test classes#33400

Merged
mccoyp merged 4 commits intoAzure:mainfrom
mccoyp:remove-azuretestcase
Dec 13, 2023
Merged

Remove deprecated test classes#33400
mccoyp merged 4 commits intoAzure:mainfrom
mccoyp:remove-azuretestcase

Conversation

@mccoyp
Copy link
Member

@mccoyp mccoyp commented Dec 5, 2023

Description

Now that the repository has moved away from using vcrpy, we don't have a reason to continue supporting Azure(Mgmt)TestCase classes; Azure(Mgmt)RecordedTestCase classes have the same core functionality, without making references to old recording infrastructure.

The last functional use of AzureMgmtTestCase is in azure-batch, since they're still migrating to track 2. Using the new test class instead still turned out to be possible (without recording capability), so that migration is handled in #33388. This PR removes the SDK's remaining, non-functional references.

Note: CI failures on this PR are unrelated to these changes.

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.

@mccoyp mccoyp added EngSys This issue is impacting the engineering system. test-enhancement labels Dec 5, 2023
@azure-sdk
Copy link
Collaborator

API change check

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

azure-messaging-webpubsubservice

@mccoyp mccoyp force-pushed the remove-azuretestcase branch from b03e643 to bd585f3 Compare December 6, 2023 00:19
@mccoyp mccoyp marked this pull request as ready for review December 6, 2023 01:32
@mccoyp mccoyp requested a review from scbedd December 6, 2023 01:33
Copy link
Member

@swathipil swathipil left a comment

Choose a reason for hiding this comment

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

looks good for SR/SB - thanks!

Copy link
Member

@scbedd scbedd left a comment

Choose a reason for hiding this comment

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

I am so excited to see this code getting cleaned up!

Is the rename of test_mgmt_documentdb to disable_test_mgmt_documentdb to make it so the file doesn't get picked up by pytest anymore? Or was this something that needed to be cleaned up?

If you intended the rename, maybe we just delete the file?

Other than that, looks reasonable to me! Thanks @mccoyp !

@mccoyp
Copy link
Member Author

mccoyp commented Dec 6, 2023

Is the rename of test_mgmt_documentdb to disable_test_mgmt_documentdb to make it so the file doesn't get picked up by pytest anymore? Or was this something that needed to be cleaned up?

If you intended the rename, maybe we just delete the file?

@scbedd Yep, that's exactly the intention. This pattern actually started a while back, when we started migrating en masse to the test proxy -- some mgmt libraries didn't have functional tests, so instead of updating imports and inheritances, we decided to exclude the tests from being loaded at testing time.

Now that it's been so long, we probably could start removing these tests instead of having the vestigial bloat. I just want to check with library owners before doing so, at which point we can just do a wider purge -- at least, that's what I was thinking.

@scbedd
Copy link
Member

scbedd commented Dec 7, 2023

Is the rename of test_mgmt_documentdb to disable_test_mgmt_documentdb to make it so the file doesn't get picked up by pytest anymore? Or was this something that needed to be cleaned up?
If you intended the rename, maybe we just delete the file?

@scbedd Yep, that's exactly the intention. This pattern actually started a while back, when we started migrating en masse to the test proxy -- some mgmt libraries didn't have functional tests, so instead of updating imports and inheritances, we decided to exclude the tests from being loaded at testing time.

Now that it's been so long, we probably could start removing these tests instead of having the vestigial bloat. I just want to check with library owners before doing so, at which point we can just do a wider purge -- at least, that's what I was thinking.

👍 I understand. I tend to think that every dev will err on the side of

eh leave it around

but it's probably just FUD. You're not wrong to toss the question at em though 💯

@mccoyp
Copy link
Member Author

mccoyp commented Dec 13, 2023

/check-enforcer override

@mccoyp mccoyp merged commit ad5ddf4 into Azure:main Dec 13, 2023
@mccoyp mccoyp deleted the remove-azuretestcase branch December 13, 2023 17:40
@akx akx mentioned this pull request Jan 20, 2026
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

EngSys This issue is impacting the engineering system. test-enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Comments