Skip to content

run samples in live test pipelines#15694

Merged
kristapratico merged 20 commits intoAzure:masterfrom
kristapratico:run-samples-ci
Dec 14, 2020
Merged

run samples in live test pipelines#15694
kristapratico merged 20 commits intoAzure:masterfrom
kristapratico:run-samples-ci

Conversation

@kristapratico
Copy link
Copy Markdown
Contributor

@kristapratico kristapratico commented Dec 8, 2020 •
edited
Loading

Resolves #13876

Adding this as an opt-in parameter: TestSamples: true and using Form Recognizer to test it. Any library that wants to run their samples in the service live test pipeline should just need to add the environment variables used in the samples to their tests.yml for it to work.

I've added this as an optional step after running tests, but not sure if that's the best spot. Let me know.

Form Recognizer run example that has the Test Samples step: https://dev.azure.com/azure-sdk/internal/_build/results?buildId=646223&view=logs&j=e4797745-1ad6-5989-d345-8ffe02e82111&t=89a0f654-1963-5cb5-5df3-6e3f7fc697ab

@kristapratico kristapratico marked this pull request as ready for review December 8, 2020 22:46
@kristapratico kristapratico self-assigned this Dec 8, 2020
@weshaggard
Copy link
Copy Markdown
Member

weshaggard commented Dec 8, 2020

FYI: @benbp

weshaggard
weshaggard reviewed Dec 8, 2020
View reviewed changes
sdk/formrecognizer/tests.yml
AZURE_FORM_RECOGNIZER_SELECTION_MARK_STORAGE_CONTAINER_SAS_URL: $(python-formrecognizer-test-selection-mark-storage-sas-url)
AZURE_FORM_RECOGNIZER_TESTING_DATA_CONTAINER_SAS_URL: $(python-formrecognizer-test-storage-testing-data-sas-url)
AZURE_FORM_RECOGNIZER_AAD_ENDPOINT: $(python-formrecognizer-test-aad-endpoint)
# EnvVars for samples to run. Mostly re-used from above values
Copy link
Copy Markdown
Member

@weshaggard weshaggard Dec 8, 2020

Choose a reason for hiding this comment

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

I think this is going in a direction that will make maintaining these very difficult. In other languages we are using arm templates for the samples to avoid the need for all these different static secrets.

I know that @seankane-msft is working to enable arm template support in the python repo and we should try to start using those for samples as well if at all possible.

Copy link
Copy Markdown
Contributor Author

@kristapratico kristapratico Dec 9, 2020

Choose a reason for hiding this comment

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

Oh nice, didn't know that we were moving towards the arm template way in Python. @seankane-msft - what's the status on that? Will that be blocking on this PR?

Copy link
Copy Markdown
Contributor

@seankane-msft seankane-msft Dec 9, 2020

Choose a reason for hiding this comment

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

Support for arm templates was added in tests.yml files. To add a deployment of arm templates you'll need a test-resources.json file and parameter DeployArmTemplate: true to your tests.yml file. The former you can probably take from the .NET/JS/Java sdks.

Copy link
Copy Markdown
Contributor Author

@kristapratico kristapratico Dec 9, 2020

Choose a reason for hiding this comment

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

Talked to Sean offline and opened an issue to move Form Recognizer to ARM templates in the near future: #15736. Confirmed that this doesn't block on this PR.

@kristapratico kristapratico mentioned this pull request Dec 9, 2020
Closed
@benbp
Copy link
Copy Markdown
Member

benbp commented Dec 9, 2020 •
edited
Loading

This is great! FYI the engsys team is trying to institute a more standardized way of running all samples as part of the smoke tests pipelines (rather than live tests). Only the js repo is currently using this new approach, though @hemanttanwar and I are working on bringing it to java as well. Seems like this PR does most of the language specific pieces, and I'd love to work towards standardizing the resource deployment and pipeline pieces alongside that for the smoke test pipeline.

Once we get test-resources.json set up in the SDK directories, it shouldn't be too much work to pull it all together.

@kristapratico
Copy link
Copy Markdown
Contributor Author

kristapratico commented Dec 9, 2020

This is great! FYI the engsys team is trying to institute a more standardized way of running all samples as part of the smoke tests pipelines (rather than live tests). Only the js repo is currently using this new approach, though @hemanttanwar and I are working on bringing it to java as well. Seems like this PR does most of the language specific pieces, and I'd love to work towards standardizing the resource deployment and pipeline pieces alongside that for the smoke test pipeline.

Once we get test-resources.json set up in the SDK directories, it shouldn't be too much work to pull it all together.

@benbp

I talked to Wes about this and my understanding was that running samples in the live pipeline served a different purpose than the smoke tests. My goal here was to alert the library owner that a particular sample is broken before we do a release. Will the smoke tests capture this and alert the owner as well? Also - let me know how I can help!

@benbp
Copy link
Copy Markdown
Member

benbp commented Dec 10, 2020

@kristapratico ah I see. Makes sense, that is a different scenario.

@scbedd
Copy link
Copy Markdown
Member

scbedd commented Dec 10, 2020

@kristapratico I really do like this.

I don't particularly like the usage of dev_setup.py, but I don't know of any instances where it would cause negative impact (installing something we don't want), as that's a different problem from what we solve here.

Is there any value to providing a tox environment that runs samples specifically? If not, then IMO this PR is good to go as is. If you do care to run samples easily from cmd, then I think what you should here is:

  1. Provide a new tox environment that runs pytest against the samples/ directory
    a. This would negate the use of dev_setup, as your tox env would use the same install config as the whl/sdist runs.
  2. Modify the PR to call setup_execute_tests against that specific environment.

Other than that feedback, looks great to me!

@kristapratico
Copy link
Copy Markdown
Contributor Author

kristapratico commented Dec 11, 2020

@scbedd

Thanks for the feedback. I believe I made the intended changes to remove usage of dev_setup and create the tox environment to run samples. Can you verify? Also lemme know if I should move anything around, not sure if my organization is wonky.

Live run after latest commit: https://dev.azure.com/azure-sdk/internal/_build/results?buildId=650411&view=logs&j=e4797745-1ad6-5989-d345-8ffe02e82111&t=89a0f654-1963-5cb5-5df3-6e3f7fc697ab

@kristapratico kristapratico merged commit 278290f into Azure:master Dec 14, 2020
@kristapratico kristapratico deleted the run-samples-ci branch December 14, 2020 23:41
This was referenced Jan 8, 2021
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@weshaggard weshaggard weshaggard left review comments

@scbedd scbedd scbedd approved these changes

@danieljurek danieljurek Awaiting requested review from danieljurek

@mitchdenny mitchdenny Awaiting requested review from mitchdenny

+1 more reviewer

@seankane-msft seankane-msft seankane-msft left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@kristapratico kristapratico

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

client library samples should be run in CI

5 participants

@kristapratico @weshaggard @benbp @scbedd @seankane-msft