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

Added PlaybackOnlyAttribute #12663

Merged
merged 2 commits into from
Jun 25, 2020

Conversation

seanmcc-msft
Copy link
Member

@seanmcc-msft seanmcc-msft commented Jun 9, 2020

We would like the ability to mark some tests as playback-only.

For example: https://github.com/Azure/azure-sdk-for-net/pull/12609/files#diff-e152777102db76590298fbda43777754R1769

Related #12609

@pakrym
Copy link
Contributor

pakrym commented Jun 9, 2020

Why do you want to ever avoid running these tests live?

@pakrym
Copy link
Contributor

pakrym commented Jun 9, 2020

Read through the test. Would we ever reenable it?

If we ever add this attribute I think it needs to take a reason for playback only.

@seanmcc-msft
Copy link
Member Author

Why do you want to ever avoid running these tests live?

In the case of ORS, we would have to take a dependency on the Storage Management SDK to set up and tear down the ORS policy with every test run. This turned out to be a non-viable solution, so instead, we are going to run the tests in playback only. We've had a few other similar cases, where it's not worth the effort to set up the infrastructure to live test a minor feature.

@tg-msft recommended we create this annotation at one point.

@kasobol-msft @amnguye

@seanmcc-msft
Copy link
Member Author

Another example is #12623. BlockBlobStorage storage accounts are legency, you can't even create one in the Azure Portal. I'm pretty sure its not possible to create one via ARM templates. To sidestep this, we gain access to a BlockBlobStorage account, create the test, record it, and we're good.

/// Attribute on test assemblies, classes, or methods that run only against playback resources.
/// </summary>
[AttributeUsage(AttributeTargets.Method | AttributeTargets.Class | AttributeTargets.Assembly, AllowMultiple = true, Inherited = true)]
public class PlaybackOnlyAttribute : NUnitAttribute, IApplyToTest
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this prevent someone from being able to re-record the test just by changing the ctor parameter?

Copy link
Member Author

Choose a reason for hiding this comment

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

Which constructor parameter are you referring to?

Copy link
Contributor

Choose a reason for hiding this comment

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

: base(async, serviceVersion, null /* RecordedTestMode.Record /* to re-record */)

Copy link
Member Author

Choose a reason for hiding this comment

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

I assume so, yes, but this is by design. The workflow to re-record would be to comment out the attribute.

/// </summary>
[AttributeUsage(AttributeTargets.Method | AttributeTargets.Class | AttributeTargets.Assembly, AllowMultiple = true, Inherited = true)]
public class PlaybackOnlyAttribute : NUnitAttribute, IApplyToTest
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a "reason" required ctor parameter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@tg-msft
Copy link
Member

tg-msft commented Jun 9, 2020

I was thinking of something a little more like a [ManualTest] attribute using the Test Category we already skip for pipeline runs plus maybe an EnvVar so folks can always skip it locally in VS without thinking. It would still be easy enough to run them live without making code changes to remove the attribute, etc.

@pakrym
Copy link
Contributor

pakrym commented Jun 9, 2020

We can also skip the test dynamically based on the existence of particular environment setting.

@tg-msft
Copy link
Member

tg-msft commented Jun 9, 2020

As long as the env setting was present in our DevOps pipeline because I'm pretty sure those machines are refurbished raspberry pi zeros and struggle getting some of these longer tests to finish before our timeout kills the whole run.

@seanmcc-msft
Copy link
Member Author

Another example of why we need this functionality - https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/storage/Azure.Storage.Blobs/tests/PageBlobClientTests.cs#L1057. It's not programmatically possible to create a managed disk storage account. That test will never run in live mode.

@pakrym
Copy link
Contributor

pakrym commented Jun 25, 2020

@seanmcc-msft what are your thoughts about #12663 (comment)?

@seanmcc-msft
Copy link
Member Author

seanmcc-msft commented Jun 25, 2020

@seanmcc-msft what are your thoughts about #12663 (comment)?

I don't think I fully understand that comment.

My objective is to have the ability to have tests that only run in playback mode, both locally and in the pipeline. These tests would rarely be re-recorded, and never run live. I never want to skip these tests in playback mode, and always skip them in recorded and live modes. To re-record, we'd comment out the annotation.

@seanmcc-msft seanmcc-msft merged commit 3e75e3b into Azure:master Jun 25, 2020
@seanmcc-msft seanmcc-msft deleted the feature/storage/PlaybackOnly branch June 25, 2020 17:32
prmathur-microsoft pushed a commit that referenced this pull request Jul 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants