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

Move common test assets out of Azure Application Configuration #3368

Merged
merged 68 commits into from
Apr 14, 2019

Conversation

conniey
Copy link
Member

@conniey conniey commented Apr 12, 2019

This is part of a couple PRs. This PR:

  • Creates a "azure-common-test" package.
  • Deletes a lot of unnecessary "factory" classes and delay providers that are not necessary.
    • The delay providers in tests can be done with reactor's VirtualTimeScheduler. We don't have to roll our own.
    • The TestName factories were just returning an instance of the class itself. 🤔 Wasn't abstracting any object creation.
  • Removes a lot of static creation methods and static context methods.
    • Having one static SdkContext, TestResourceNamer, etc. will make it impossible to run test classes in parallel.
  • Creates a TestBase class.

Related to #3208.

@conniey conniey self-assigned this Apr 12, 2019
@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

Copy link
Member

@alzimmermsft alzimmermsft left a comment

Choose a reason for hiding this comment

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

Given these

/**
* Base class for running live and playback tests using {@link InterceptorManager}.
*/
public abstract class TestBase {
Copy link
Member

Choose a reason for hiding this comment

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

Should we also add placeholders for BeforeClass and AfterClass? These could allow performance wins with the tests instead of having to instantiate common build for every test.

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 thought about it, but the existing TestBase had a BeforeClass and AfterClass that didn't add value. I'm not sure how performance could be improved with having the base class with a no-op BeforeClass that then calls the implementing class.

Copy link
Member

Choose a reason for hiding this comment

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

That was left a little too vague on my part, less on the stub methods being useful now but more on they would allow optimizations. For example the App Config unit tests create a new builder, run an authenticate credentials, and construct a new client for each test. That could just get done once during the beforeClass call.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh I see what you're saying. Looking into the BeforeClass annotation... unfortunately it has to be a static method. So, it doesn't lend itself to having a stub method that can be overridden in an implementing class. :/

Copy link
Member

@JonathanGiles JonathanGiles left a comment

Choose a reason for hiding this comment

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

LGTM - just a package rename suggestion

@conniey conniey merged commit 2d2066c into Azure:master Apr 14, 2019
@conniey conniey deleted the fixes3208 branch April 14, 2019 05:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants