Skip to content
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.

Sign up for GitHub

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

chore(StorageManager): typos and minor code cleanups #5009

Merged
merged 5 commits into from
May 14, 2022
Merged

chore(StorageManager): typos and minor code cleanups #5009

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
71045e8
chore(StorageManager): typos and minor code cleanups
keturn May 8, 2022
132d900
test(StorageManager): remove redundant createFolder
keturn May 13, 2022
5ff4f1c
test(StorageManager): ensure each test has its own save name
keturn May 13, 2022
f89b7dc
test(StorageManager): add TODO for lowered Mockito strictness
keturn May 14, 2022
e5c9c75
test(StorageManager): factor out lengthy saveName
keturn May 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 26 additions & 33 deletions ...ne-tests/src/test/java/org/terasology/engine/persistence/internal/StorageManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,22 @@
// Copyright 2021 The Terasology Foundation
// Copyright 2022 The Terasology Foundation
// SPDX-License-Identifier: Apache-2.0
package org.terasology.engine.persistence.internal;

import com.google.common.collect.Lists;
import org.joml.Vector3f;
import org.joml.Vector3i;
import org.joml.Vector3ic;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.MethodOrderer;
import org.junit.jupiter.api.Order;
import org.junit.jupiter.api.Tag;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.TestInfo;
import org.junit.jupiter.api.TestMethodOrder;
import org.junit.jupiter.api.io.TempDir;
import org.mockito.ArgumentMatchers;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.junit.jupiter.MockitoExtension;
import org.mockito.junit.jupiter.MockitoSettings;
import org.mockito.quality.Strictness;
import org.terasology.engine.TerasologyTestingEnvironment;
import org.terasology.engine.core.PathManager;
import org.terasology.engine.core.bootstrap.EntitySystemSetupUtil;
Expand Down Expand Up @@ -46,7 +48,6 @@
import org.terasology.engine.world.chunks.ChunkProvider;
import org.terasology.engine.world.chunks.blockdata.ExtraBlockDataManager;
import org.terasology.engine.world.chunks.internal.ChunkImpl;
import org.terasology.engine.world.internal.WorldInfo;
import org.terasology.gestalt.assets.ResourceUrn;
import org.terasology.gestalt.assets.management.AssetManager;
import org.terasology.gestalt.module.ModuleEnvironment;
Expand All @@ -55,13 +56,12 @@
import org.terasology.unittest.stubs.EntityRefComponent;
import org.terasology.unittest.stubs.StringComponent;

import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
import java.lang.reflect.Method;
import java.nio.file.Path;
import java.util.Arrays;
import java.util.List;
import java.util.UUID;

import static com.google.common.truth.Truth.assertWithMessage;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
Expand All @@ -71,13 +71,12 @@

@TestMethodOrder(MethodOrderer.OrderAnnotation.class)
@Tag("TteTest")
@ExtendWith(MockitoExtension.class)
@MockitoSettings(strictness = Strictness.LENIENT)
public class StorageManagerTest extends TerasologyTestingEnvironment {

public static final String PLAYER_ID = "someId";
public static final Vector3ic CHUNK_POS = new Vector3i(1, 2, 3);

private static File temporaryFolder;

private ModuleEnvironment moduleEnvironment;
private ReadWriteStorageManager esm;
private EngineEntityManager entityManager;
Expand All @@ -91,21 +90,16 @@ public class StorageManagerTest extends TerasologyTestingEnvironment {
private RecordAndReplayUtils recordAndReplayUtils;
private RecordAndReplayCurrentStatus recordAndReplayCurrentStatus;

@BeforeAll
static void createFolder() throws IOException {
File createdFolder = File.createTempFile("junit", "", null);
createdFolder.delete();
createdFolder.mkdir();
temporaryFolder = createdFolder;
}

@BeforeEach
public void setup(@TempDir Path tempHome) throws Exception {
public void setup(TestInfo testInfo) throws Exception {
super.setup();
PathManager.getInstance().useOverrideHomePath(tempHome);
savePath = PathManager.getInstance().getSavePath("testSave");

assert !Files.isRegularFile(tempHome.resolve("global.dat"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this assertion no longer relevant?

Copy link
Member

@pollend pollend May 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should use the junit test function not assert. assertFalse()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

savePath = PathManager.getInstance().getSavePath("testSave-" +
testInfo.getTestMethod().map(Method::getName).orElseGet(() -> UUID.randomUUID().toString()));

assertWithMessage("Leftover files in %s", savePath)
.that(savePath.resolve("global.dat").toFile().exists())
.isFalse();

entityManager = context.get(EngineEntityManager.class);
moduleEnvironment = mock(ModuleEnvironment.class);
Expand Down Expand Up @@ -134,7 +128,7 @@ public void setup(@TempDir Path tempHome) throws Exception {
Client client = createClientMock(PLAYER_ID, character);
NetworkSystem networkSystem = mock(NetworkSystem.class);
when(networkSystem.getMode()).thenReturn(NetworkMode.NONE);
when(networkSystem.getPlayers()).thenReturn(Arrays.asList(client));
when(networkSystem.getPlayers()).thenReturn(List.of(client));
context.put(NetworkSystem.class, networkSystem);

AssetManager assetManager = context.get(AssetManager.class);
Expand All @@ -147,7 +141,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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this still required or can it be removed entirely?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

context.put(WorldProvider.class, worldProvider);
}

Expand All @@ -163,8 +157,7 @@ private EntityRef createClientEntity(EntityRef charac) {
ClientComponent clientComponent = new ClientComponent();
clientComponent.local = true;
clientComponent.character = charac;
EntityRef clientEntity = entityManager.create(clientComponent);
return clientEntity;
return entityManager.create(clientComponent);
}

@Test
Expand Down Expand Up @@ -267,7 +260,7 @@ public void testStoreAndRestoreChunkStore() {
chunk.setBlock(0, 0, 0, testBlock);
chunk.markReady();
ChunkProvider chunkProvider = mock(ChunkProvider.class);
when(chunkProvider.getAllChunks()).thenReturn(Arrays.asList(chunk));
when(chunkProvider.getAllChunks()).thenReturn(List.of(chunk));
CoreRegistry.put(ChunkProvider.class, chunkProvider);

esm.waitForCompletionOfPreviousSaveAndStartSaving();
Expand All @@ -287,8 +280,8 @@ public void testChunkSurvivesStorageSaveAndRestore() throws Exception {
chunk.setBlock(0, 4, 2, testBlock2);
chunk.markReady();
ChunkProvider chunkProvider = mock(ChunkProvider.class);
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same question

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above

CoreRegistry.put(ChunkProvider.class, chunkProvider);
boolean storeChunkInZips = true;

Expand Down Expand Up @@ -318,7 +311,7 @@ public void testEntitySurvivesStorageInChunkStore() throws Exception {
chunk.setBlock(0, 0, 0, testBlock);
chunk.markReady();
ChunkProvider chunkProvider = mock(ChunkProvider.class);
when(chunkProvider.getAllChunks()).thenReturn(Arrays.asList(chunk));
when(chunkProvider.getAllChunks()).thenReturn(List.of(chunk));
CoreRegistry.put(ChunkProvider.class, chunkProvider);
EntityRef entity = entityManager.create();
long id = entity.getId();
Expand Down Expand Up @@ -349,7 +342,7 @@ public void testEntitySurvivesStorageInChunkStore() throws Exception {


@Test
public void testCanSavePlayerWithoutUnloading() throws Exception {
public void testCanSavePlayerWithoutUnloading() {
esm.waitForCompletionOfPreviousSaveAndStartSaving();
esm.finishSavingAndShutdown();

Expand Down
85 changes: 42 additions & 43 deletions engine/src/main/java/org/terasology/engine/persistence/internal/ReadWriteStorageManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2021 The Terasology Foundation
// Copyright 2022 The Terasology Foundation
// SPDX-License-Identifier: Apache-2.0
package org.terasology.engine.persistence.internal;

Expand Down Expand Up @@ -86,30 +86,30 @@ public final class ReadWriteStorageManager extends AbstractStorageManager
private final Lock worldDirectoryReadLock = worldDirectoryLock.readLock();
private final Lock worldDirectoryWriteLock = worldDirectoryLock.writeLock();
private SaveTransaction saveTransaction;
private Config config;
private SystemConfig systemConfig;
private final Config config;
private final SystemConfig systemConfig;

/**
* Time of the next save in the format that {@link System#currentTimeMillis()} returns.
*/
private Long nextAutoSave;
private boolean saveRequested;
private ConcurrentMap<Vector3ic, CompressedChunkBuilder> unloadedAndUnsavedChunkMap = Maps.newConcurrentMap();
private ConcurrentMap<Vector3ic, CompressedChunkBuilder> unloadedAndSavingChunkMap = Maps.newConcurrentMap();
private ConcurrentMap<String, EntityData.PlayerStore> unloadedAndUnsavedPlayerMap = Maps.newConcurrentMap();
private ConcurrentMap<String, EntityData.PlayerStore> unloadedAndSavingPlayerMap = Maps.newConcurrentMap();
private final ConcurrentMap<Vector3ic, CompressedChunkBuilder> unloadedAndUnsavedChunkMap = Maps.newConcurrentMap();
private final ConcurrentMap<Vector3ic, CompressedChunkBuilder> unloadedAndSavingChunkMap = Maps.newConcurrentMap();
private final ConcurrentMap<String, EntityData.PlayerStore> unloadedAndUnsavedPlayerMap = Maps.newConcurrentMap();
private final ConcurrentMap<String, EntityData.PlayerStore> unloadedAndSavingPlayerMap = Maps.newConcurrentMap();


private EngineEntityManager privateEntityManager;
private final EngineEntityManager privateEntityManager;
private EntitySetDeltaRecorder entitySetDeltaRecorder;
private RecordAndReplaySerializer recordAndReplaySerializer;
private RecordAndReplayUtils recordAndReplayUtils;
private RecordAndReplayCurrentStatus recordAndReplayCurrentStatus;
private final RecordAndReplaySerializer recordAndReplaySerializer;
private final RecordAndReplayUtils recordAndReplayUtils;
private final RecordAndReplayCurrentStatus recordAndReplayCurrentStatus;
/**
* A component library that provides a copy() method that replaces {@link EntityRef}s which {@link EntityRef}s
* that will use the privateEntityManager.
*/
private ComponentLibrary entityRefReplacingComponentLibrary;
private final ComponentLibrary entityRefReplacingComponentLibrary;

public ReadWriteStorageManager(Path savePath, ModuleEnvironment environment, EngineEntityManager entityManager, BlockManager blockManager,
ExtraBlockDataManager extraDataManager, RecordAndReplaySerializer recordAndReplaySerializer,
Expand Down Expand Up @@ -182,7 +182,7 @@ private void addGlobalStoreBuilderToSaveTransaction(SaveTransactionBuilder trans
@Override
public void deactivatePlayer(Client client) {
EntityRef character = client.getEntity().getComponent(ClientComponent.class).character;
PlayerStoreBuilder playerStoreBuilder = createPlayerStore(client, character);
PlayerStoreBuilder playerStoreBuilder = createPlayerStore(character);
EntityData.PlayerStore playerStore = playerStoreBuilder.build(getEntityManager());
deactivateOrDestroyEntityRecursive(character);
unloadedAndUnsavedPlayerMap.put(client.getId(), playerStore);
Expand Down Expand Up @@ -291,15 +291,15 @@ private void addPlayersToSaveTransaction(SaveTransactionBuilder saveTransactionB
// If there is a newer undisposed version of the player,we don't need to save the disposed version:
unloadedAndSavingPlayerMap.remove(client.getId());
EntityRef character = client.getEntity().getComponent(ClientComponent.class).character;
saveTransactionBuilder.addLoadedPlayer(client.getId(), createPlayerStore(client, character));
saveTransactionBuilder.addLoadedPlayer(client.getId(), createPlayerStore(character));
}

for (Map.Entry<String, EntityData.PlayerStore> entry : unloadedAndSavingPlayerMap.entrySet()) {
saveTransactionBuilder.addUnloadedPlayer(entry.getKey(), entry.getValue());
}
}

private PlayerStoreBuilder createPlayerStore(Client client, EntityRef character) {
private PlayerStoreBuilder createPlayerStore(EntityRef character) {
LocationComponent location = character.getComponent(LocationComponent.class);
Vector3f relevanceLocation;
if (location != null) {
Expand Down Expand Up @@ -375,7 +375,7 @@ private void addGameManifestToSaveTransaction(SaveTransactionBuilder saveTransac
WorldGenerator worldGenerator = CoreRegistry.get(WorldGenerator.class);
if (worldGenerator != null) {
WorldConfigurator worldConfigurator = worldGenerator.getConfigurator();
Map<String, Component> params = worldConfigurator.getProperties();
var params = worldConfigurator.getProperties();
gameManifest.setModuleConfigs(worldGenerator.getUri(), params);
}

Expand All @@ -397,7 +397,6 @@ public void update() {
} else if (isSavingNecessary()) {
startAutoSaving();
}

}

private boolean isRunModeAllowSaving() {
Expand All @@ -407,45 +406,45 @@ private boolean isRunModeAllowSaving() {

private void startSaving() {
logger.info("Saving - Creating game snapshot");
PerformanceMonitor.startActivity("Saving");
ComponentSystemManager componentSystemManager = CoreRegistry.get(ComponentSystemManager.class);
for (ComponentSystem sys : componentSystemManager.getAllSystems()) {
sys.preSave();
}
try (var ignored = PerformanceMonitor.startActivity("Saving")) {
ComponentSystemManager componentSystemManager = CoreRegistry.get(ComponentSystemManager.class);
for (ComponentSystem sys : componentSystemManager.getAllSystems()) {
sys.preSave();
}

saveRequested = false;
saveTransaction = createSaveTransaction();
saveThreadManager.offer(saveTransaction);
saveRequested = false;
saveTransaction = createSaveTransaction();
saveThreadManager.offer(saveTransaction);

if (recordAndReplayCurrentStatus.getStatus() == RecordAndReplayStatus.NOT_ACTIVATED) {
saveGamePreviewImage();
}
if (recordAndReplayCurrentStatus.getStatus() == RecordAndReplayStatus.NOT_ACTIVATED) {
saveGamePreviewImage();
}

for (ComponentSystem sys : componentSystemManager.getAllSystems()) {
sys.postSave();
for (ComponentSystem sys : componentSystemManager.getAllSystems()) {
sys.postSave();
}
}
PerformanceMonitor.endActivity();
entitySetDeltaRecorder = new EntitySetDeltaRecorder(this.entityRefReplacingComponentLibrary);
logger.info("Saving - Snapshot created: Writing phase starts");
}

private void startAutoSaving() {
logger.info("Auto Saving - Creating game snapshot");
PerformanceMonitor.startActivity("Auto Saving");
ComponentSystemManager componentSystemManager = CoreRegistry.get(ComponentSystemManager.class);
for (ComponentSystem sys : componentSystemManager.getAllSystems()) {
sys.preAutoSave();
}
try (var ignored = PerformanceMonitor.startActivity("Auto Saving")) {
ComponentSystemManager componentSystemManager = CoreRegistry.get(ComponentSystemManager.class);
for (ComponentSystem sys : componentSystemManager.getAllSystems()) {
sys.preAutoSave();
}

saveTransaction = createSaveTransaction();
saveThreadManager.offer(saveTransaction);
saveTransaction = createSaveTransaction();
saveThreadManager.offer(saveTransaction);

for (ComponentSystem sys : componentSystemManager.getAllSystems()) {
sys.postAutoSave();
}
for (ComponentSystem sys : componentSystemManager.getAllSystems()) {
sys.postAutoSave();
}

scheduleNextAutoSave();
PerformanceMonitor.endActivity();
scheduleNextAutoSave();
}
entitySetDeltaRecorder = new EntitySetDeltaRecorder(this.entityRefReplacingComponentLibrary);
logger.info("Auto Saving - Snapshot created: Writing phase starts");
}
Expand Down
Loading