chore(StorageManager): typos and minor code cleanups #5009
Conversation
keturn
added
Size: S
| @@ -147,7 +145,7 @@ public void setup(@TempDir Path tempHome) throws Exception { | |||
|
|
|||
| context.put(ChunkProvider.class, mock(ChunkProvider.class)); | |||
| WorldProvider worldProvider = mock(WorldProvider.class); | |||
| when(worldProvider.getWorldInfo()).thenReturn(new WorldInfo()); | |||
| // when(worldProvider.getWorldInfo()).thenReturn(new WorldInfo()); | |||
There was a problem hiding this comment.
is this still required or can it be removed entirely?
There was a problem hiding this comment.
This is the thing that spooks me about mocks.
Mockito told me that not all the mocks were used for all the tests. For instance, some of the tests don't use the NetworkManager mocks. A couple of these lines weren't used by any of the tests—those are the ones I've commented out.
But I have a hunch that they were needed at some point or they wouldn't have been written. What if the next test needs them? Or what if something somewhere in the depths of one of these systems shifts which methods it calls when, and now this gets called again?
So I left it a little messy. With the intent that we'll come back to this after #5010 is merged and take another look at what kind of HeadlessEnvironment these tests need.
There was a problem hiding this comment.
in that case, please add a TODO there so that it's clear, why this is commented-out but not removed a.k.a what's unclear about it and what's necessary before it can be clarified and either re-introduced or removed.
| when(chunkProvider.getAllChunks()).thenReturn(Arrays.asList(chunk)); | ||
| when(chunkProvider.getChunk(ArgumentMatchers.any(Vector3ic.class))).thenReturn(chunk); | ||
| when(chunkProvider.getAllChunks()).thenReturn(List.of(chunk)); | ||
| // when(chunkProvider.getChunk(ArgumentMatchers.any(Vector3ic.class))).thenReturn(chunk); |
| savePath = PathManager.getInstance().getSavePath("testSave"); | ||
|
|
||
| assert !Files.isRegularFile(tempHome.resolve("global.dat")); |
There was a problem hiding this comment.
why is this assertion no longer relevant?
There was a problem hiding this comment.
this should use the junit test function not assert. assertFalse()
There was a problem hiding this comment.
I think the intention of that assert was to make sure that we aren't re-using a directory that already has stuff in it. I took it out because it was already guaranteed unique by @TempDir, and because I removed this TempDir stuff because there's already TempDir stuff setting up the PathManager in TerasologyTestingEnvironment.
But your comment did prompt me to take another look, and I found that I had broken things here, I just hadn't gotten caught yet. (global.dat would be under savePath, not tempHome) This is a BeforeEach, but TTE was only doing the path setup BeforeAll.
I've fixed it now so every test gets its own save name, regardless of whether the PathManager has been reset between methods or not.
Responding to code inspections and suggestions.
keturn
force-pushed
the
chore/storage-manager-tidy
branch
from
e6a099a
to
71045e8
Compare
It's handled by the superclass.
| @@ -147,7 +145,7 @@ public void setup(@TempDir Path tempHome) throws Exception { | |||
|
|
|||
| context.put(ChunkProvider.class, mock(ChunkProvider.class)); | |||
| WorldProvider worldProvider = mock(WorldProvider.class); | |||
| when(worldProvider.getWorldInfo()).thenReturn(new WorldInfo()); | |||
| // when(worldProvider.getWorldInfo()).thenReturn(new WorldInfo()); | |||
There was a problem hiding this comment.
in that case, please add a TODO there so that it's clear, why this is commented-out but not removed a.k.a what's unclear about it and what's necessary before it can be clarified and either re-introduced or removed.
| when(chunkProvider.getAllChunks()).thenReturn(Arrays.asList(chunk)); | ||
| when(chunkProvider.getChunk(ArgumentMatchers.any(Vector3ic.class))).thenReturn(chunk); | ||
| when(chunkProvider.getAllChunks()).thenReturn(List.of(chunk)); | ||
| // when(chunkProvider.getChunk(ArgumentMatchers.any(Vector3ic.class))).thenReturn(chunk); |
|
Comments on comments, hmm? Alright, how about this: I've un-commented those lines—the test's mockito strictness is already lowered to lenient, so that won't break anything—and I've added a TODO comment with an explanation for the lenient setting. Thank you for the review! I'll merge once Jenkins approves this latest change. |
Responding to code inspections and suggestions.
Some cleanup to reduce distractions while working on StorageManager for #4800.
Might have some conflicts with that branch, but both these branches are pretty small, shouldn't be too much trouble to resolve.