Make Lookup instantiatable with a default constructor retaining the original behavior#750
Merged
Merged
Conversation
Contributor
Author
|
I've created this merely as a draft PR because of the lack of tests for the new functionality. Let's discuss the implementation approach in the meantime :) |
the original behavior.
bruno-garcia
reviewed
Aug 7, 2019
location configurable. Also made the prefixes of env vars, system properties and JNDI names containing the individual configuration properties configurable. Got rid of the superfluous SentryConfiguration interface and PropertiesConfigurationprovider.
Contributor
Author
|
Note that I've started using Mockito in the new tests, for better or worse. IMHO, it is a de-facto industry standard and doesn't allow for the insanity of mocking system classes like jmockit does. IMHO, that's for good reasons. |
bruno-garcia
approved these changes
Aug 14, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This should help with the testability of majority of other parts of the codebase because we can start introducing overloads accepting the new
SentryConfigurationinstance instead of statically callingLookup(which implements theSentryConfigurationinterface).SentryConfigurationbeing an interface makes it trivially mockable without resorting to the ugliness needed to mock JNDI look up et al.Note that I didn't implement any new tests yet to check the new capabilities ofLookupnor did I update any existing code or tests to take advantage of it. That said,Lookupitself uses the new code and the testsuite seems to be passing with it.