feat: async functions to make sure blocks/chunks are loaded #66

keturn · 2021-09-08T06:56:01Z

This provides a more powerful version of the forceAndWaitForGeneration helper method. The new makeBlocksRelevant and makeChunksRelevant methods load all required chunks to cover the given region.

Depends on:

feat(RelevanceSystem): addRelevanceEntity returns the relevant region MovingBlocks/Terasology#4902

Reviewers:

Which things do you think most need documentation?
ChunkRegionFutureTest contains one test.
- Is it clear to you what it is testing and what information is relevant?
- What additional tests would you need in order to feel comfortable working on this code and confident in its operation?

DarkWeird · 2021-09-08T07:06:32Z

src/main/java/org/terasology/moduletestingenvironment/ModuleTestingEnvironment.java

@@ -311,14 +366,21 @@ public boolean runWhile(long gameTimeTimeoutMs, Supplier<Boolean> f) {

        while (f.get() && !timedOut) {
            Thread.yield();
+            if (Thread.currentThread().isInterrupted()) {


ehm.... how current thread can handle this if current thread is interrupted??
there should be try-catch - isn't it?

“Interrupted” in Java threads is a funny thing. As I understand it, Thread#interrupt() does not cause a sudden jump in the code like a POSIX signal in Linux. It only sets a status on the thread to say "hello, I would like you to be interrupted now, please stop—if you feel like it."

Some standard blocking functions like sleep and read notice this and throw an exception. But if we're just crunching numbers in a loop, there might not be anything that would make us take notice of the interrupt request unless we check for it ourselves.

DarkWeird · 2021-09-08T07:32:23Z

src/main/java/org/terasology/moduletestingenvironment/ModuleTestingEnvironment.java

            for (TerasologyEngine terasologyEngine : engines) {
-                terasologyEngine.tick();
+                boolean keepRunning = terasologyEngine.tick();
+                if (!keepRunning && terasologyEngine == host) {


Is host can be shutdown normally?

I guess someone could write a test that includes shutdown as expected behavior, but that's pretty different from the MTE tests I've seen so far.

keturn · 2021-10-08T20:11:03Z

The required engine PR was merged, so this does build now.

I'm afraid that as much fun as I had writing it, I don't expect this is readable/maintainable for most people:

ModuleTestingEnvironment/src/main/java/org/terasology/moduletestingenvironment/ChunkRegionFuture.java

Lines 53 to 62 in cc33e46

    
           BiFunction<ChunkRegionListener, Vector3ic, BlockRegionc> naivelyMakesForSize; 
        
           BiFunction<ChunkRegionListener, Vector3ic, BlockRegionc> makesForSize; 
        
           naivelyMakesForSize = (listener, size) -> relevanceSystem.addRelevanceEntity(entity, size, listener); 
        
           makesForSize = Functional.<ChunkRegionListener, Vector3ic, BlockRegionc> 
        
                           enwiden(ChunkRegionFuture::regionSizeCorrector) 
        
                   .apply(naivelyMakesForSize); 
        
           Function<ChunkRegionListener, BlockRegionc> makeChunksRelevant = 
        
                   listener -> makesForSize.apply(listener, sizeInChunks);

so I think we had better find another way to write that without the higher-order function gymnastics before merging this one.

src/main/java/org/terasology/moduletestingenvironment/ModuleTestingEnvironment.java

keturn · 2021-10-19T16:29:33Z

amidst all the distractions in the test log, there is this:

[Chunk-Processing-Reactor] ERROR o.t.e.w.c.p.ChunkProcessingPipeline - Cannot run java.util.concurrent.ExecutorCompletionService$QueueingFuture@6a1cf258 because queue is full

keturn · 2021-10-19T20:21:39Z

huh. Only saw that particular "queue full" message once; the later test runs don't have it.

Now this is in the unfortunate position of Works On My Machine, Fails in CI. Which I don't like, but it's also status quo—that's why we have @Tag("MteTest").

While I'd be very happy to see that fixed, I would also be okay with merging this without the Integration Tests stage passing, so that other tests can start using these new functions.

keturn · 2021-10-19T21:28:02Z

src/test/java/org/terasology/moduletestingenvironment/ChunkRegionFutureTest.java

+    @Test
+    void createChunkRegionFuture(Context context, ModuleTestingHelper mte) {
+        ChunkRegionFuture chunkRegionFuture = ChunkRegionFuture.create(context, center, sizeInChunks);


This test uses MTEExtension so it can get a Context with a working RelevanceSystem and all. But is it something that could also make use of a stub RelevanceSystem to test something worthwhile?

Might be good to have, especially given that we haven't yet fixed the problems with MTE tests on our CI system.

Why don't use entityManager, relevanceSystem instead?
Context using looks like you want to inject something in context.

Which problem?

Really, I want to use the injection system. Some classes, like Test cases and Terasology Systems, get to make references to the things in Context without cluttering up their method signatures.

But I don't know how to get things injected like that when constructing instances of other classes.

Overall, though, I only ended up using two things from the Context, so yeah it's probably worth the cost of one additional argument to remove that confusion.

fixed to remove the Context reference.

keturn · 2021-10-24T17:21:30Z

new MTE makeBlocksRelevant methods

DarkWeird

Tested at FlexibleMovement module - it works (only one engine creating and waiting many generation waiting after every test world purge)

Many checkstyles issue about FIXME and TODO. I guess it will fixed after this PR.
Several JavaDoc's checkstyle issues

DarkWeird · 2021-10-25T09:02:05Z

src/main/java/org/terasology/moduletestingenvironment/ChunkRegionFuture.java

+        return Collections.unmodifiableSet(loadedChunks);
+    }
+
+    /** The entity defining the relevance region. */


check style:
no @return

I don't have any good ideas about what to do here.

What more information would you like to see about this?

DarkWeird · 2021-10-25T09:02:32Z

src/main/java/org/terasology/moduletestingenvironment/ChunkRegionFuture.java

+
+    @SuppressWarnings("unused")
+    public BlockRegionc getChunkRegion() {
+        return chunks;


those chunks should be dense?

I don't know what you mean by that.

DarkWeird · 2021-10-25T09:03:58Z

src/main/java/org/terasology/moduletestingenvironment/ChunkRegionFuture.java

+
+        @Override
+        public void onChunkIrrelevant(Vector3ic pos) {
+            // FIXME: Document why this IS, in fact, called regularly.


onChunkIrrelevant happens when chunk leave relevant region (entity with Location and Relevant region move away)

That would make sense, but try uncommenting the logger.warn here and running the ChunkRegionFutureTest.

Nothing moves in that test, but onChunkIrrelevant sure does get called!

DarkWeird · 2021-10-25T09:04:45Z

src/main/java/org/terasology/moduletestingenvironment/ModuleTestingEnvironment.java

        WorldProvider worldProvider = hostContext.get(WorldProvider.class);
        if (worldProvider.isBlockRelevant(blockPos)) {
-            return;
+            return null;


Can you describe this at javadoc's @return line?

Hmm. I didn't really think about this one.

This was previously a void method, so no existing code is using a return value from it.

When I changed this implementation to be on top of makeBlocksRelevant, I thought it would make sense to return the value from that (as seen below on line 277).

But it has this early-escape clause where it doesn't call makeBlocksRelevant at all.

What would make most sense here?

Leave it always returning void?

Remove the isBlockRelevant check, and always return the value from makeBlocksRelevant?

Finding no good way to explain that, I've reverted this back to having no return value.

DarkWeird · 2021-10-25T09:06:44Z

src/main/java/org/terasology/moduletestingenvironment/ModuleTestingEnvironment.java

@@ -399,8 +454,6 @@ public long getSafetyTimeoutMs() {
     *
     * The safety timeout exists to prevent indefinite execution in Jenkins or long IDE test runs, and should be
     * adjusted as needed so that tests pass reliably in all environments.
-     *
-     * @param safetyTimeoutMs


I moved more of the above documentation to be within the @param block, is that what you were looking for?

DarkWeird · 2021-10-25T09:13:28Z

src/test/java/org/terasology/moduletestingenvironment/ChunkRegionFutureTest.java

+    @Test
+    void createChunkRegionFuture(Context context, ModuleTestingHelper mte) {
+        ChunkRegionFuture chunkRegionFuture = ChunkRegionFuture.create(context, center, sizeInChunks);


Why don't use entityManager, relevanceSystem instead?
Context using looks like you want to inject something in context.

Which problem?

keturn · 2021-10-28T23:14:21Z

@DarkWeird when I mentioned “we haven't yet fixed the problems with MTE tests on our CI system,” I was referring to the way the Integration Tests have not been passing for a long time: https://jenkins.terasology.io/teraorg/job/Terasology/job/Modules/job/M/job/ModuleTestingEnvironment/job/develop/

keturn · 2021-10-28T23:39:46Z

I forwarded some questions about what to do about the JavaDoc to #documentation.

keturn

I also pushed some javadoc syntax corrections.

keturn · 2021-11-02T02:20:14Z

src/main/java/org/terasology/moduletestingenvironment/ChunkRegionFuture.java

+        return Collections.unmodifiableSet(loadedChunks);
+    }
+
+    /** The entity defining the relevance region. */


I don't have any good ideas about what to do here.

What more information would you like to see about this?

keturn · 2021-11-02T02:21:31Z

src/main/java/org/terasology/moduletestingenvironment/ModuleTestingEnvironment.java

@@ -399,8 +454,6 @@ public long getSafetyTimeoutMs() {
     *
     * The safety timeout exists to prevent indefinite execution in Jenkins or long IDE test runs, and should be
     * adjusted as needed so that tests pass reliably in all environments.
-     *
-     * @param safetyTimeoutMs


I moved more of the above documentation to be within the @param block, is that what you were looking for?

DarkWeird reviewed Sep 8, 2021

View reviewed changes

keturn mentioned this pull request Sep 15, 2021

chore: make runUntil/runWhile return values consistent #68

Merged

keturn changed the title ~~WIP: async functions to make sure blocks/chunks are loaded~~ feat: async functions to make sure blocks/chunks are loaded Oct 18, 2021

keturn marked this pull request as ready for review October 18, 2021 15:50

keturn added the Type: Improvement Request for or addition/enhancement of a feature label Oct 18, 2021

keturn commented Oct 19, 2021

View reviewed changes

src/main/java/org/terasology/moduletestingenvironment/ModuleTestingEnvironment.java Outdated Show resolved Hide resolved

keturn commented Oct 19, 2021

View reviewed changes

DarkWeird requested changes Oct 25, 2021

View reviewed changes

keturn commented Nov 2, 2021

View reviewed changes

keturn force-pushed the spike/loadChunks branch 2 times, most recently from 9574f55 to 3697f80 Compare November 2, 2021 02:47

keturn requested a review from DarkWeird November 2, 2021 02:53

DarkWeird previously approved these changes Nov 2, 2021

View reviewed changes

keturn added 2 commits November 2, 2021 09:51

feat: async functions to make sure blocks/chunks are loaded

3d2bcc7

chore(javadoc): add escaping where necessary for valid syntax

075e8ae

keturn dismissed DarkWeird’s stale review via 075e8ae November 2, 2021 16:53

keturn force-pushed the spike/loadChunks branch from 3697f80 to 075e8ae Compare November 2, 2021 16:53

keturn merged commit bd1b59c into develop Nov 2, 2021

keturn deleted the spike/loadChunks branch November 2, 2021 16:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: async functions to make sure blocks/chunks are loaded #66

feat: async functions to make sure blocks/chunks are loaded #66

keturn commented Sep 8, 2021 •

edited

Loading

DarkWeird Sep 8, 2021

keturn Sep 8, 2021

DarkWeird Sep 8, 2021

keturn Sep 8, 2021

keturn commented Oct 8, 2021

keturn commented Oct 19, 2021 •

edited

Loading

keturn commented Oct 19, 2021

keturn Oct 19, 2021

DarkWeird Oct 25, 2021

keturn Oct 28, 2021

keturn Oct 28, 2021

keturn commented Oct 24, 2021

DarkWeird left a comment •

edited

Loading

DarkWeird Oct 25, 2021

keturn Nov 2, 2021

DarkWeird Oct 25, 2021

keturn Oct 28, 2021

DarkWeird Oct 25, 2021

keturn Oct 28, 2021

DarkWeird Oct 25, 2021

keturn Oct 28, 2021

keturn Nov 2, 2021

DarkWeird Oct 25, 2021

keturn Nov 2, 2021

DarkWeird Oct 25, 2021

keturn commented Oct 28, 2021 •

edited

Loading

keturn commented Oct 28, 2021

keturn left a comment

keturn Nov 2, 2021

keturn Nov 2, 2021

feat: async functions to make sure blocks/chunks are loaded #66

feat: async functions to make sure blocks/chunks are loaded #66

Conversation

keturn commented Sep 8, 2021 • edited Loading

Depends on:

Reviewers:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keturn commented Oct 8, 2021

keturn commented Oct 19, 2021 • edited Loading

keturn commented Oct 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keturn commented Oct 24, 2021

DarkWeird left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keturn commented Oct 28, 2021 • edited Loading

keturn commented Oct 28, 2021

keturn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keturn commented Sep 8, 2021 •

edited

Loading

keturn commented Oct 19, 2021 •

edited

Loading

DarkWeird left a comment •

edited

Loading

keturn commented Oct 28, 2021 •

edited

Loading