Skip to content

[Test Proxy] Add test recording fixture - #24848

Merged
Laia Patiño (laiapat) merged 13 commits into
Azure:mainfrom
laiapat:proxy-fixture
Jul 21, 2022
Merged

[Test Proxy] Add test recording fixture#24848
Laia Patiño (laiapat) merged 13 commits into
Azure:mainfrom
laiapat:proxy-fixture

Conversation

@laiapat

@laiapat Laia Patiño (laiapat) commented Jun 14, 2022

Copy link
Copy Markdown
Member

Description

This takes an alternative approach to recording and playing back tests: using a fixture instead of a wrapping decorator.

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.

@laiapat Laia Patiño (laiapat) added the EngSys This issue is impacting the engineering system. label Jun 14, 2022
@azure-sdk

Azure SDK Bot (azure-sdk) commented Jun 29, 2022

Copy link
Copy Markdown
Collaborator

API change check

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

azure-data-tables
azure-keyvault-keys

@laiapat
Laia Patiño (laiapat) force-pushed the proxy-fixture branch 3 times, most recently from d76096f to 13ede80 Compare July 9, 2022 00:48
Comment thread sdk/conftest.py Outdated
Comment thread sdk/conftest.py Outdated

@scbedd Scott Beddall (scbedd) left a comment

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.

This is looking really good McCoy. We are missing some integration checking, and that's pretty much it right?

@laiapat

Copy link
Copy Markdown
Member Author

Yep, we're almost there! I just need to add async functionality, make a few tweaks to the variable recorder for easier use, and work out any integration and invocation kinks (like applying the fixture to tests from a class or session level). I'll mark this ready for review and provide an update when it's there 🙂

Comment thread sdk/conftest.py Outdated
@scbedd

Copy link
Copy Markdown
Member

In terms of integrating this PR with the rest of the repo. Perhaps we maintain the existing recorded_by_proxy so that we can merge this and make forward progress while migrating previous implementations.

@laiapat

Copy link
Copy Markdown
Member Author

In terms of integrating this PR with the rest of the repo. Perhaps we maintain the existing recorded_by_proxy so that we can merge this and make forward progress while migrating previous implementations.

I completely agree. That raises a point about the main blocker for marking this ready for review: the variables fixture isn't compatible with parameterized tests. So, for example, most Key Vault tests can't use the variable_recorder fixture.

That isn't an immediate issue since: 1. KV tests don't use variables today, and 2. the recorded_by_proxy decorator will still be available as an alternative. But it's something I'd like to figure out, though I haven't been able to after a few days of trying.

Since the decorator approach to recording will still be supported, should we try to get this merged sooner rather than later, once the small packaging issue is resolved? The ML team is hoping to migrate their tests soon, and they would need this recording fixture to make that happen. Scott Beddall (@scbedd) Laurent Mazuel (@lmazuel)

@lmazuel

Copy link
Copy Markdown
Member

Talked to @mccoyp offline, thumbs up. ;)

@laiapat
Laia Patiño (laiapat) marked this pull request as ready for review July 20, 2022 21:25
@laiapat

Copy link
Copy Markdown
Member Author

/azp run python - keyvault - ci

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@laiapat

Copy link
Copy Markdown
Member Author

Build verifying that conftest.py gets loaded correctly for servicebus (the only other service that has a dev_requirements.txt excluding azure-sdk-tools -- specifically in a perf-specific file): https://dev.azure.com/azure-sdk/public/_build/results?buildId=1722981&view=results

@scbedd Scott Beddall (scbedd) left a comment

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.

This is great McCoy. Maybe Anna Tisch (@annatisch) will pick something out that I'm not seeing, but this looks super solid.

@annatisch Anna Tisch (annatisch) left a comment

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.

LGTM :)

@laiapat
Laia Patiño (laiapat) merged commit 0c156c9 into Azure:main Jul 21, 2022
@laiapat
Laia Patiño (laiapat) deleted the proxy-fixture branch July 21, 2022 18:13
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants