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

DropwizardTestSupport should only override the app's ConfigurationSou… #2720

Conversation

natalie-zamani
Copy link
Member

…rceProvider if one is explicitly provided in a constructor (#2719)

Problem:

See #2719

Solution:

Made DropwizardTestSupports ConfigurationSourceProvider override nullable, and only override the app's ConfigurationSourceProvider if one is explicitly provided via a constructor.

Result:

This will fix the behavior of DropwizardTestSupport in tests when the app sets a different ConfigurationSourceProvider.

@natalie-zamani natalie-zamani force-pushed the fix-test-support-config-source-provider-overriding-behavior branch from aaa8834 to 9a072be Compare April 5, 2019 15:54
…rceProvider if one is explicitly provided in a constructor (dropwizard#2719)
@natalie-zamani natalie-zamani force-pushed the fix-test-support-config-source-provider-overriding-behavior branch from 9a072be to 8dcf226 Compare April 5, 2019 18:04
@natalie-zamani
Copy link
Member Author

@nickbabcock Anything else to consider with this one? Or does this approach seem tenable?

Copy link
Contributor

@nickbabcock nickbabcock left a comment

Choose a reason for hiding this comment

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

Yes, this looks to be the proper way for a test to override an app's ConfigurationSourceProvider 👍

@joschi joschi added the bug label Apr 6, 2019
@joschi joschi added this to the 2.0.0 milestone Apr 6, 2019
@joschi joschi merged commit 6c17257 into dropwizard:master Apr 6, 2019
@natalie-zamani natalie-zamani deleted the fix-test-support-config-source-provider-overriding-behavior branch April 8, 2019 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants