Added logging for non-fabric mods with tests. #907
Conversation
Co-authored-by: Douglas Fischer <[email protected]> Co-authored-by: Robin Claesson <[email protected]> Co-authored-by: Acuadragon100 <[email protected]> Co-authored-by: Erik Vinblad <[email protected]>
| StringBuilder outputText = new StringBuilder(); | ||
|
|
||
| for (Path nonFabricMod : nonFabricMods) { | ||
| outputText.append("\n\t- ").append(nonFabricMod.getFileName()); |
There was a problem hiding this comment.
Question: Whats the behaviour when a mod has a none fabric mod contained within?
E.g you have mods/mymod.jar/notamod.jar where mymod.jar is a valid fabric mod, and within its fabric.mod.json file it points to nodamod.jar
I think it might be confusing for it to print out that it found a none mod jar notamod.jar
There was a problem hiding this comment.
It only adds to the non-fabric-mod list from ModScanTask::computeJarFile, so it will ignore any embedded mods as well as directory mods which do not seem to load anyways.
| Mockito.when(launcher.getEnvironmentType()).thenReturn(EnvType.CLIENT); | ||
| Mockito.when(launcher.isDevelopment()).thenReturn(false); | ||
|
|
||
| Method setLauncher = FabricLauncherBase.class.getDeclaredMethod("setLauncher", FabricLauncher.class); |
There was a problem hiding this comment.
I dont think using reflection is tests is a good idea especially against code that can be changed, this makes the tests harder to maintain and more likely to break with future changes.
You can use the @VisibleForTesting annotation if needed.
You have done a really good job writing unit tests for this, considering that the original code was never desgined to be unit tested. If this was new code that I was writing it would likely be written with unit testing in mind.
There was a problem hiding this comment.
Should be fixed now.
| * Calls dumpNonFabricMods using reflection, so we don't need to make the class | ||
| * and method public. |
There was a problem hiding this comment.
As above, I would make it public and mark it with @VisibleForTesting, its all within the impl package so we dont expect anyone to even attempt to use this.
There was a problem hiding this comment.
Should be fixed now.
| @BeforeEach | ||
| public void setUp() throws NoSuchMethodException, InvocationTargetException, IllegalAccessException { | ||
| GameProvider provider = mock(); | ||
| Mockito.when(provider.getBuiltinMods()).thenReturn(Collections.emptyList()); |
There was a problem hiding this comment.
Nit: Static import Mockito.when so this can just become when(provider. You already do it for mock()
There was a problem hiding this comment.
Should be fixed now.
|
Just realised that the fact that the non-fabric mod list is not synchronised might cause issues, will commit a fix shortly. |
Acuadragon100
force-pushed
the
log-non-fabric-mods
branch
from
c03f3fd to
3044a67
Compare
* feat(FabricMC#644): add @VisibleForTesting for the feat impl method * Use static import for Mockito::when. * Use @VisibleForTesting instead of reflection. Co-authored-by: Douglas Fischer <[email protected]> Co-authored-by: wenqic <[email protected]> Co-authored-by: Acuadragon100 <[email protected]>
Co-authored-by: Douglas Fischer <[email protected]>
|
We're done for now. Ready for another review :) |
| @@ -35,6 +35,7 @@ | |||
| import java.util.function.Consumer; | |||
| import java.util.stream.Collectors; | |||
|
|
|||
| import com.google.common.annotations.VisibleForTesting; | |||
There was a problem hiding this comment.
| import com.google.common.annotations.VisibleForTesting; | |
| import org.jetbrains.annotations.VisibleForTesting; |
| StringBuilder outputText = new StringBuilder(); | ||
|
|
||
| for (Path nonFabricMod : nonFabricMods) { | ||
| outputText.append("\n\t- ").append(nonFabricMod.getFileName()); |
Added functionality to log all non-fabric-mod jar files, along with some tests.
Closes #644