Update gradle plugins

[RoiEXLab]

Not that there's the following issue that seems to break the build, but actually doesn't: GradleUp/shadow#336
This will probably be fixed in a future version

[codecov-io]

Codecov Report

Merging #2722 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #2722      +/-   ##
============================================
+ Coverage     20.08%   20.08%   +<.01%     
  Complexity     5757     5757              
============================================
  Files           820      820              
  Lines         73743    73717      -26     
  Branches      12278    12237      -41     
============================================
- Hits          14810    14809       -1     
+ Misses        56796    56776      -20     
+ Partials       2137     2132       -5

Impacted Files	Coverage Δ	Complexity Δ
...ne/framework/map/download/DownloadCoordinator.java	6.75% <0%> (-1.36%)	1% <0%> (ø)
src/main/java/games/strategy/thread/LockUtil.java	73.68% <0%> (-0.46%)	13% <0%> (ø)
...s/strategy/triplea/attachments/UnitAttachment.java	41.84% <0%> (-0.33%)	324% <0%> (ø)
...mes/strategy/triplea/delegate/MustFightBattle.java	66.68% <0%> (-0.18%)	350% <0%> (ø)
.../games/strategy/triplea/delegate/MoveDelegate.java	48.32% <0%> (-0.14%)	59% <0%> (ø)
...y/triplea/delegate/StrategicBombingRaidBattle.java	52% <0%> (-0.11%)	24% <0%> (ø)
...y/engine/data/gameparser/XmlGameElementMapper.java	97.67% <0%> (-0.03%)	19% <0%> (ø)
...va/games/strategy/engine/framework/GameRunner.java	0% <0%> (ø)	0% <0%> (ø)	⬇️
.../strategy/triplea/ai/proAI/ProNonCombatMoveAI.java	0% <0%> (ø)	0% <0%> (ø)	⬇️
...ava/games/strategy/triplea/ui/ProductionPanel.java	0% <0%> (ø)	0% <0%> (ø)	⬇️
... and 32 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 069cd90...6b945cd. Read the comment docs.

[ssoloff]

I think it would be clearer to just find another way to add duplicate units to the collection. Using the UnitType#create(PlayerID) overload seems a better choice here:

diff --git a/src/test/java/games/strategy/triplea/delegate/MoveDelegateTest.java b/src/test/java/games/strategy/triplea/delegate/MoveDelegateTest.java
index 00f68f8ed..d20ae3f4d 100644
--- a/src/test/java/games/strategy/triplea/delegate/MoveDelegateTest.java
+++ b/src/test/java/games/strategy/triplea/delegate/MoveDelegateTest.java
@@ -7,8 +7,8 @@ import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collection;
-import java.util.Collections;
 import java.util.List;
 
 import org.junit.jupiter.api.BeforeEach;
@@ -50,10 +50,8 @@ public class MoveDelegateTest extends DelegateTest {
     final Route route = new Route();
     route.setStart(egypt);
     route.add(eastAfrica);
-    final List<Unit> units = armour.create(1, british);
-    // Wrapping this line into an unmodifiable List
-    // is neccessary to avoid the errorprone error
-    units.addAll(Collections.unmodifiableList(units));
+    final Unit unit = armour.create(british);
+    final List<Unit> units = Arrays.asList(unit, unit);
     final String results = delegate.move(units, route);
     assertError(results);
   }

[ssoloff]

Note that this is triggering a lot of new warnings. Most of them look legitimate. However, one in particular, ClassCanBeStatic, is triggering a lot of false negatives in test code. The JUnit @Nested annotation requires nested tests to be inner classes (non-static) rather than nested classes (static), so we have no choice here.

Does Error Prone offer an annotation mechanism to ignore individual warnings? Or is there a way to have Error Prone ignore this specific warning for all classes that have a Test suffix?

[RoiEXLab]

@ssoloff The latest version offers a way to exclude classes with filenames that match a given regular expression.
I disabled error prone for all files inside src/test and src/integ_test folder.
There is a legacy way of doing it that by annotating classes with @SuppressWarnings("<errorname>"), in this case : @SuppressWarnings("ClassCanBeStatic"), but it's really not pretty, especially because custom compilers like eclipse will warn you of this uneccessary annotation

[RoiEXLab]

@ssoloff I also disabled ReferenceEquality checks for now.
I believe there are many occasions where this check results in a false positive.
We can change that if I'm wrong about this.

[RoiEXLab]

But nevertheless there are indeed some very good suggestions from error prone I will try to adress in a seperate PR

[ssoloff]

I'd recommend not turning off ReferenceEquality for now. I agree there may be some false negatives related to enums (which should probably be reported back to Error Prone), but I think we should take our time evaluating them rather than simply disabling it and possibly forgetting that we did so. If we agree it is not providing much value, then we can disable it en masse.

Disabling Error Prone for all tests is probably overkill. We can just turn off ClassCanBeStatic for unit test code with the following change:

diff --git a/build.gradle b/build.gradle
index 112a3bfcf..a053fe14f 100644
--- a/build.gradle
+++ b/build.gradle
@@ -68,7 +68,7 @@ sourceCompatibility = 1.8
 targetCompatibility = 1.8
 
 tasks.withType(JavaCompile) {
-    options.compilerArgs += [ '-Xlint:all', '-XepExcludedPaths:.*\\/src\\/(integ_)?test\\/.*', '-Xep:ReferenceEquality:OFF' ]
+    options.compilerArgs += [ '-Xlint:all' ]
     options.incremental = true
     options.encoding = 'UTF-8'
 }
@@ -303,6 +303,10 @@ checkstyleTest {
     source sourceSets.test.output.resourcesDir
 }
 
+compileTestJava {
+    options.compilerArgs += [ '-Xep:ClassCanBeStatic:OFF' ]
+}
+
 afterEvaluate {
     def junitPlatformTest = tasks.junitPlatformTest
 

It would be great if Error Prone supported disabling individual rules by path (so we could apply the filter to only *Test classes), but that doesn't appear possible in the current version.

[RoiEXLab]

@ssoloff I applied your patch and redid the changes to avoid the error prone error.
Self-merging...