vorburger commented on a change in pull request #830:
URL: https://github.com/apache/fineract/pull/830#discussion_r429559026
##########
File path:
fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/StaffTest.java
##########
@@ -99,9 +101,10 @@ public void testStaffCreateMaxNameLength() {
map.put("firstname", Utils.randomNameGenerator("michael_", 42));
map.put("lastname", Utils.randomNameGenerator("Doe_", 46));
- StaffHelper.createStaffWithJson(requestSpec, responseSpec, new
Gson().toJson(map));
+ StaffHelper.createStaffWithJson(requestSpec,
responseSpecForValidationError, new Gson().toJson(map));
Review comment:
this seems a bit suspicious to me - so this test if failing, but you are
just making the response expect a validation error? Shouldn't we... fix this
test, if we re-enable it, instead of cheating to get the test to pass?
Re-enabling these tests could be done in a separate PR before this one, if you
like (but don't have to).
##########
File path: fineract-provider/build.gradle
##########
@@ -276,7 +278,7 @@ configurations {
}
tasks.withType(JavaCompile) {
- options.compilerArgs += ["-Xlint:unchecked","-Xlint:cast","-Werror"] //
TODO FINERACT-959 (gradually) enable -Xlint:all (see "javac -help -X")
+ options.compilerArgs += ["-Xlint:unchecked","-Xlint:cast"] // TODO
FINERACT-959 (gradually) enable -Xlint:all (see "javac -help -X")
Review comment:
wait waaaaat... why are we regressing on FINERACT-959 to progress with
FINERACT-822? That doesn't seem right, to me - and worth a discussion...
:smiling_imp: I suspect that you may be doing this because Error Prone gives
warnings? IMHO we should either fix them all, as part of this - or change EP's
config to shut up about particular ones which are obviously wrong (similar to
what I have done in another project, have you seen the link in FINERACT-822)...
How many are there?
##########
File path: fineract-provider/build.gradle
##########
@@ -435,6 +437,9 @@ dependencies {
tomcat "org.apache.tomcat:tomcat:9.0.34@zip"
}
+dependencies {
+ errorprone "com.google.errorprone:error_prone_core:2.3.3"
Review comment:
```suggestion
errorprone "com.google.errorprone:error_prone_core:2.3.4"
```
##########
File path:
fineract-provider/src/main/java/org/apache/fineract/interoperation/util/MathUtil.java
##########
@@ -87,6 +87,7 @@ public static Long abs(Long value) {
/** @return calculates minimum of the two values considering null values
* @param notNull if true then null parameter is omitted, otherwise
returns null */
+ @SuppressWarnings("NullTernary")
Review comment:
whoa whoa - slow down, the point is to FIX all problems - not to
introduce a tool that tells us about problems, and then just make the tool shut
up! I have not looked closely, but just a quick glance at
https://errorprone.info/bugpattern/NullTernary makes it seem to me like this is
trying to telly us about a real bug? As above, if it's definitely a false
positive, then put a detailed analysis explanation in an inline comment...
##########
File path:
fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/StaffTest.java
##########
@@ -183,13 +186,17 @@ public void testStaffUpdate() {
Assert.assertEquals(mobileNo, changes.get("mobileNo"));
}
+ @Test
+ @Ignore
Review comment:
can you raise a JIRA for the failure and then link it here using
`@Ignore // TODO FINERACT-123` syntax?
##########
File path:
fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/variableinstallments/VariableInstallmentsFlatHelper.java
##########
@@ -201,6 +201,7 @@ private static ArrayList createModifyMap(String date) {
return toReturn;
}
+ @SuppressWarnings("MisusedWeekYear")
Review comment:
this seems suspicious - are you sure suppressing this is correct, and
not actually hiding a real bug? (I haven't had a closer look, I'm hoping you'll
do that...) If you are completely sure, then each `@SuppressWarnings` should
have an inline comment with a reasonably detailed explanation why that is the
right thing to do.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
