-
Notifications
You must be signed in to change notification settings - Fork 1
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
AuthDBProperties - bug fix to get resource file #91
Conversation
962faf4
to
d105c0e
Compare
5d5781b
to
9dcaae5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like what you've done, but I have a couple questions.
|
||
private String server = null; | ||
private String service = null; | ||
private String userName = null; | ||
private String userPass = null; | ||
private String propertyFile = null; | ||
private Properties properties = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it make more sense to pass "properties" as an argument to initDataSourceProperties(), since we call it in the constructor(s) and we must instantiate properties before calling it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not likely; it is initialized by two different methods and those methods are specific to the type of constructor called. Thereafter, it's available to the init method. This is optimal already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, I went down that road before and ended up where it is now. For example, if it gets passed, we get pseudo-code like this:
initProps( propertyFile ) {
if propertyFile == null
properties = loadResourceProperties
else
properties = loadPropertiesFile( propertyFile )
...
}
So, putting the initializers in the constructors avoids this null check. I hope that helps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant pass it the Properties object itself -- the result of loadPropertiesFile()
or loadResourceProperties()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yes, that's possible. The next consideration is how easy the class is to test. As it is, those methods are easier to test (or use in tests). I guess that's why it is this way, rather than those loading methods be void methods. At some point while developing tests, it was easier if they return Properties. It's reasonable to request this, but it could entail more work to change tests as well. Not sure we really gain much? Perhaps we can pair on this rather than play github comment tennis?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I get your point correctly, it seems your requesting changes like the following:
// lines 84 - 85 are:
Properties props = new Properties();
props.load(iStream);
// they are replaced by using the instance var
properties.load(iStream);
I'll try that and see what happens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, the new commit might be what your proposing. (Leaving it as a new commit for review purposes only; if it looks OK, it can be squashed later.)
// Use private method to get the server.conf properties | ||
Method m = AuthDBProperties.class.getDeclaredMethod("loadDataSourceProperties", String.class); | ||
|
||
Method m = AuthDBProperties.class.getDeclaredMethod("loadPropertyResource"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I misunderstanding this? It seems like this line is calling the class method to get data, and then we are matching below (testConstructor, testConstructorWithString) if the data we got using the same method, but called differently (directly rather than with "getDeclaredMethod"), is equal. I would always expect the same method to give the same result, given the same data ...
It looks like the test was that way before this PR ... just thinking it might be better to check the loaded properties against hardcoded known value strings here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the data in the properties file changes without changing any hard-coded values in the test, the test breaks. There are various ways to do this, for sure, and this one suffices to get the job done. If using fixed values is required to merge this, we can do that, but it just shifts that data into two places (the resource file and the test, both of which only exist in the tests).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yet another way to do this is to assertNotNull
, but that's less specific (although less fragile and does not depend on checking specific values).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok leaving this as is;
It's true there is a maintenance cost incurred that if you change the data being used for tests, you have to change the hardcoded data in the tests ... but in my experience it is still better to test for actual values than test a method's results against ... the same method's results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the tests could get direct access to the resource file without using the class... Or if the tests could create the resource file dynamically... but that's not possible, right? Or it's more work. Either way, there are tradeoffs and resource files have already given me a run around, in more ways than one (better explained in person than go on and on about it here).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latest commit gets the resource file "directly" in the tests.
@@ -1,4 +1,4 @@ | |||
SERVER=libdb.stanford.edu | |||
SERVER=db.example.com |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
d56bb17
to
866f12e
Compare
Fix #90
This fix was working in an integration test (using it in the TB-scripts to run conversions).