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

Add support for making recordings external to the repository #19322

Merged
merged 10 commits into from
Oct 20, 2022

Conversation

benbp
Copy link
Member

@benbp benbp commented Oct 10, 2022

Fixes Azure/azure-sdk-tools#4257

This adds support to the recording package to work with test proxy asset sync, which will allow the test recordings to be moved out of the go repository into https://github.com/Azure/azure-sdk-assets.

The primary behavior change is: if an assets.json file is found in the current test service directory or any parent, the recording framework will request external assets via the test proxy during test setup. If there is no assets.json, then pre-existing recordings will be used instead. This change should support running tests via run_tests.ps1 or invoking go test directly.

I also added some minor improvements to the testing scripts.

  • Improve error and pipeline handling in run_tests.ps1
  • Add test proxy default assets directory to gitignore
  • Support test proxy external asset mode for test recordings

@benbp benbp added the Central-EngSys This issue is owned by the Engineering System team. label Oct 10, 2022
@benbp benbp self-assigned this Oct 10, 2022
@benbp benbp requested a review from scbedd October 10, 2022 18:29
@benbp benbp added the EngSys This issue is impacting the engineering system. label Oct 10, 2022
@benbp
Copy link
Member Author

benbp commented Oct 10, 2022

Looks like windows tests are failing due to some path handling differences with the OS. Investigating...

@benbp
Copy link
Member Author

benbp commented Oct 10, 2022

@jhendrixMSFT @richardpark-msft do you anticipate a future requirement to define configs for recording assets in directories below the service directory (e.g. deeper than sdk/keyvault/azkeys)? That is to say, will we need more granularity with recordings than the main package? I've skipped that functionality for now assuming it won't be needed.

@jhendrixMSFT
Copy link
Member

In azblob there are tests in subdirectories. Does this mean the feature is required?

@benbp
Copy link
Member Author

benbp commented Oct 10, 2022

In azblob there are tests in subdirectories. Does this mean the feature is required?

It's a weird scenario I'm thinking of where you would have tests in a sub-directory, and an asset configuration for those test recordings in the sub-directory, but you invoke the tests from a parent directory (either with ./... or like ./container/...). Let me dig back into it.

@jhendrixMSFT
Copy link
Member

We definitely need go test ./... from the azblob directory to work.

@benbp benbp force-pushed the benbp/proxy-assets branch from 862d8b7 to 8f484c8 Compare October 12, 2022 02:19
@benbp
Copy link
Member Author

benbp commented Oct 12, 2022

Alright, the latest update is much more flexible with the working directory of go test and the asset config path relative to the recording directory (anywhere at or above), as well as fixing some windows file and root detection issues. It does require that tests be run from within a git repository, i.e. this won't work if you have downloaded the repo code without .git, but I don't anticipate this will be an issue.

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.

This looks great. Thank you so much @benbp

@benbp
Copy link
Member Author

benbp commented Oct 18, 2022

/azp run go - internal - ci

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@benbp benbp force-pushed the benbp/proxy-assets branch from a36b85e to 822befe Compare October 19, 2022 21:23
@benbp benbp force-pushed the benbp/proxy-assets branch 2 times, most recently from 78fec7f to 058d6d1 Compare October 19, 2022 22:14
@benbp benbp force-pushed the benbp/proxy-assets branch from 058d6d1 to 0e588a4 Compare October 20, 2022 20:30
@benbp benbp merged commit 2827e1e into Azure:main Oct 20, 2022
@benbp benbp deleted the benbp/proxy-assets branch October 20, 2022 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Central-EngSys This issue is owned by the Engineering System team. EngSys This issue is impacting the engineering system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate asset-sync w/ azure-sdk-for-go
5 participants