test(MTE): replace IsolatedMTEExtension with JUnit's TestInstance.Lifecycle #5039

keturn · 2022-06-01T20:50:30Z

This removes the @IsolatedMTEExtension variety of MTE test in favor of using JUnit's TestInstance.Lifecycle to manage sharing things between test methods, as proposed in Terasology/ModuleTestingEnvironment#75.

This simplifies things and fixes #5038.

The first commit here a43627f contains a test case that reproduced the failure described in #5038.

In practice, this is mostly compatible with existing MTE tests, but they may need some adjustments. For specifics, see #5039 (comment)

test: add NetworkMode setting to MTE #5037 would be nice to have, as it would let us separate this bug from the "port already in use" failure of trying to start two servers on the same port.
test: reducing logspam #5022 would be nice to have, as both PRs edit MTEExtension and conflicts are unavoidable.

How to test

Run MTE tests of modules. Note that the modules must import MTE from org.terasology.integrationtest to use this code; check for module PRs linked to #5010.

keturn · 2022-06-02T23:33:38Z

I decided to go ahead and rip out the @IsolatedMTEExtension support. Having two modes was okay, but having our two modes multiplied by JUnit's two modes for test instance lifecycle was getting messy.

The good news is that I don't see anything in @Terasology that actually used @IsolatedMTEExtension.

The bad news is that JUnit's default test lifecycle is PER_METHOD, the equivalent of our Isolated mode. That effectively switches the mode of all existing MTE tests.

What's the impact of that?

Primarily test speed.

It's also possible that some tests were written to depend on sharing state between test cases. I haven't checked.

To address the speed concern, we can look at things like #5024 and #5029.

We could also change existing tests to use PER_CLASS lifecycle where appropriate. That's not necessarily a 100% drop-in compatible replacement, as those tests may have been depending on per-class behavior for our engines but per-method behavior for other instance fields.

The other incompatible change is that MTE will no longer provide parameter injection for @BeforeAll static methods. But if anyone was using it, it was probably buggy, so I don't think expect that to be much of a loss.

All in all, there might be some tests we need to update, but there aren't that many MTE tests total. I think it'll be an acceptable cost for the incompatible change.

keturn · 2022-06-02T23:40:07Z

Of course, the main question here should be: Did it fix the overlapping engines bug?

I think so. None of MTE's tests with either PER_CLASS or PER_METHOD lifecycles trigger it any more. I'm not 100% certain, because there are things I don't understand about some of JUnit's ExtensionContext details, but having simplified things seems to be mostly-compatible with our existing tests and less error-prone.

…instead

# Conflicts: # engine-tests/src/main/java/org/terasology/engine/integrationenvironment/jupiter/MTEExtension.java

engine-tests/src/main/java/org/terasology/engine/integrationenvironment/Engines.java engine-tests/src/main/java/org/terasology/engine/integrationenvironment/jupiter/MTEExtension.java

keturn · 2022-06-06T22:39:41Z

[…] separate this bug from the "port already in use" failure of trying to start two servers on the same port.

I tested the reproduction patch from this branch in combination with #5041, and I'm glad to say the test is still effective with server ports out of the equation:

expected: org.terasology.engine.core.TerasologyEngine@19630e3b
but was : org.terasology.engine.core.TerasologyEngine@795971b4

That eases some concern I had: while troubleshooting earlier, I had explicit "count how many instances are alive at a time" checking. Knowing it can show up in tests without that additional accounting makes me feel okay about going ahead with this branch as-is.

...e-tests/src/main/java/org/terasology/engine/integrationenvironment/jupiter/MTEExtension.java

skaldarnar · 2022-06-07T20:57:05Z

...ava/org/terasology/engine/integrationenvironment/MTEExtensionTestWithPerMethodLifecycle.java

    }

    @Test
    @Order(2)
-    public void thingsStillExistForSecondTest() {
+    public void thingsDoNotPolluteSecondTest() {
        List<EntityRef> entities = Lists.newArrayList(entityManager.getEntitiesWith(DummyComponent.class));
        // There should be one entity, created by the @BeforeAll method


There is no longer a @BeforeAll method, but only the @BeforeEach.

Suggested change

// There should be one entity, created by the @BeforeAll method

// There should be one entity, created by the @BeforeEach method for this one test

Also needs to be updated in

Terasology/engine-tests/src/test/java/org/terasology/engine/integrationenvironment/MTEExtensionTestWithPerMethodLifecycle.java

Line 46 in 1b230c2

// Create some entity to be shared by all the tests.

and

Terasology/engine-tests/src/test/java/org/terasology/engine/integrationenvironment/MTEExtensionTestWithPerMethodLifecycle.java

Line 63 in 1b230c2

// There should be one entity, created by the @BeforeAll method

The other two places also needs updates, couldn't suggest them via the UI...

Yes, and now that I'm having another look at that test class I've discovered I'm confused about how it should go—

There's some disorganization among the tests. Not always clear where to find (or add) a test case with the various combinations of features, and some things seem redundant.

The problem with this class is that I re-labelled it as a "does not share data between tests" thing, but the seenNames field it's checking in the second part is shared because it's static, so it's not making a lot of sense and it's not real clear what it should do.

One option would be to drop this entire test class and claim it is covered by https://github.com/MovingBlocks/Terasology/blob/1b230c2354a540a54b485d3a333d7627babb8d0a/engine-tests/src/test/java/org/terasology/engine/integrationenvironment/LifecyclePerMethodInjectionTest.java

I think the difference between this test and that one is that this messes with the state of the EntityManager, and the InjectionTest one only meddles with the Context.

Is that close enough, or is Context too much of a weird special case compared to the other managers?

Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>

skaldarnar · 2022-06-07T21:05:35Z

I ran MTE tests locally (gradlew integrationTest) with the test/moveMTE branches checked out where available.

Details

❯ gooey workspace:status --offline
workspace
  engine ts-omega                          test/mteEngineOv… 1b230c23
modules
  module AdditionalFruits                  develop           1bce2103
  module AdditionalItemPipes               develop           2db193e8
  module AdditionalRails                   develop           5ed9df62
  module AdditionalVegetables              develop           65a2b23a
  module AdvancedRails                     develop           0b0793c4
  module AdventureAssets                   develop           415dbe8a
  module Alchemy                           develop           f91b292f
  module AlterationEffects                 develop           d61f03a7
  module Anatomy                           develop           63200f5f
  module AnotherWorld                      develop           36175fb4
  module AnotherWorldPlants                develop           caa20b3f
  module Apiculture                        develop           c438de08
  module BasicCrafting                     develop           88ff60ec
  module AlchemyPlantGenerator             develop           cbc54adb
  module BlockDetector                     test/moveMTE      c616f482
  module BlockNetwork                      develop           17320dad
  module BlockPicker                       develop           85a69070
  module Books                             develop           009f9e39
  module Breathing                         develop           6040d5d1
  module CakeLie                           develop           922de279
  module Behaviors                         develop           72e10a3a
  module BiomesAPI                         develop           d5283a8f
  module Caves                             develop           88b6a7e4
  module ChangingBlocks                    develop           60a0bb75
  module CheatsForAll                      develop           93d631bd
  module ChiselBlocks                      develop           fd965bd2
  module ChrisVolume1OST                   develop           630bb917
  module ChrisVolume2OST                   develop           a4d55c06
  module Climbables                        develop           4a5500be
  module CombatSystem                      develop           85c39e22
  module CommonWorld                       develop           7dc6a86d
  module Compass                           develop           11985d9e
  module ComputerMonitors                  develop           c82609b7
  module Cooking                           develop           a30499bd
  module Cities                            develop           8a6b0a90
  module ClimateConditions                 develop           a7f0c60b
  module CoreAdvancedAssets                develop           56daaab5
  module CoreAssets                        develop           77898ffc
  module CoreRendering                     develop           32df2959
  module CoreSampleGameplay                develop           176903b1
  module CoreWorlds                        develop           33eea24b
  module CustomOreGen                      develop           66d252eb
  module Dialogs                           develop           d5104470
  module Drops                             develop           4352c75b
  module Durability                        develop           b9afc570
  module DynamicCities                     test/moveMTE      ea318dd2
  module Economy                           develop           ceadb44c
  module EdibleFlora                       develop           3fb585a2
  module EdibleSubstance                   develop           85a65ac2
  module DamagingBlocks                    develop           abe23b30
  module EquipmentSmithing                 develop           80e7cbee
  module EventualSkills                    develop           0432e9c8
  module Exoplanet                         develop           0ba5d3e7
  module Explosives                        develop           541fb168
  module FallingBlocks                     develop           89ea765d
  module Fences                            develop           a98fa52c
  module Equipment                         develop           a92feaae
  module FlowingLiquids                    develop           419e6c7f
  module Fluid                             develop           3b580fad
  module FluidComputerIntegration          develop           2298b714
  module FlyingIslands                     develop           9eafb459
  module FunnyBlocks                       develop           ce7dda76
  module Furnishings                       develop           3069462c
  module FlexiblePathfinding               develop           490b2218
  module GenericRocks                      develop           b0dcea0c
  module Genome                            develop           c0146ad4
  module GooKeeper                         develop           f1583b46
  module Gooey                             develop           7ad6112c
  module GooeyDefence                      develop           09e16687
  module GooeysQuests                      develop           38e68c07
  module GrowingFlora                      develop           4735fa2a
  module HumanoidCharacters                develop           1b193fda
  module Hunger                            develop           beb2e4aa
  module IRLCorp                           develop           a872c7de
  module InGameHelp                        develop           3be1f506
  module InGameHelpAPI                     develop           465e9fe7
  module Inferno                           develop           5cca810e
  module Health                            test/moveMTE      1e91cedd
  module HjerlHede                         develop           f78a355e
  module Inventory                         develop           979c2469
  module ItemPipes                         test/moveMTE      b49b1baa
  module ItemRendering                     develop           8735faab
  module JoshariasSurvival                 develop           59263219
  module Journal                           develop           7d777df3
  module KComputers                        develop           23c3d3d3
  module LegacyMusic                       develop           f114e82c
  module LightAndShadow                    develop           c9b3bcaf
  module LightAndShadowResources           develop           d9bac213
  module Lost                              develop           65505ea1
  module Machines                          develop           79031ebc
  module Kallisti                          develop           15e04d42
  module Lakes                             develop           ab018551
  module ManualLaborEventualSkills         develop           febc4631
  module MarkovChains                      develop           5a2fbe86
  module MasterOfOreon                     develop           30985551
  module Maze                              develop           209a61c4
  module MedievalCities                    develop           61136594
  module MetalRenegades                    develop           814e898f
  module ManualLabor                       develop           c301ddf5
  module Minesweeper                       develop           e2c8af33
  module Minimap                           develop           ae9eccba
  module MobileBlocks                      develop           2451eaef
  module ModularComputers                  develop           99c8c7bb
  module MoreLights                        develop           47e4b2b0
  module MultiBlock                        develop           760cc728
  module Minerals                          develop           e3650d9e
  module NameGenerator                     test/moveMTE      c5b6da26
  module Notifications                     develop           c77b54c7
  module OreGeneration                     develop           4f6c174f
  module Oreons                            develop           298f483f
  module ParadIce                          develop           f029fac1
  module PhysicalStats                     develop           de087a2d
  module MusicDirector                     develop           b46a7edc
  module PolyWorld                         develop           52dbef90
  module Portals                           develop           d7415c8e
  module PotentialEnergyDevices            develop           9380a2d7
  module Potions                           develop           25312fbc
  module Projectile                        develop           c44e99ca
  module Rails                             test/moveMTE      1454361d
  module PlantPack                         develop           c3acf4e1
  module Scenario                          develop           47b83f1d
  module Seasons                           develop           4744a208
  module SegmentedPaths                    develop           06398b18
  module Sensors                           develop           da644ad5
  module ShatteredPlanes                   develop           d710bc3f
  module Signalling                        develop           34a34be8
  module Sample                            develop           68e1729f
  module Smithing                          develop           cbf03bd3
  module Soils                             develop           3fa52128
  module SoundyGenetics                    develop           7e8563a8
  module Spawning                          develop           169d4e69
  module StaticCities                      develop           94a82941
  module StructuralResources               develop           913595d1
  module SimpleFarming                     test/moveMTE      f282636d
  module SubstanceMatters                  develop           77f4c07d
  module Tasks                             test/moveMTE      372cdf6e
  module Thirst                            develop           2480f923
  module Valentines                        develop           b1f4f14d
  module WeatherManager                    develop           7867014d
  module WildAnimals                       develop           93eaf762
  module StructureTemplates                develop           7446a49e
  module Workstation                       develop           da9ce88b
  module WorkstationCrafting               develop           13d25466
  module WorkstationInGameHelp             develop           20a2d60d
  module WorldlyTooltip                    develop           afb680f1
  module WorldlyTooltipAPI                 develop           fc0f727c
  module WildAnimalsGenome                 develop           834c3808
  module Xmas                              develop           d494a779

I saw test failures for the following modules:

Rails on test/moveMTE (6 tests, 1 failure)

org.opentest4j.AssertionFailedError: expected: <rails:rails> but was: <engine:unloaded>

BlockDetector on test/moveMTE (4 tests, 2 failures)

java.lang.NullPointerException
  at org.terasology.blockdetector.systems.BlockDetectorSystemImpl.detectBlocks(BlockDetectorSystemImpl.java:262)

Behaviors on develop (all 227 failing)

jdrueckert · 2022-06-07T21:39:17Z

I didn't see the Rails failure in CI on Terasology/Rails#90 - the PR was green.

The two BlockDetector failures I saw, but they seem to be fixed by Terasology/BlockDetector#14 (which has conflicts after merging Terasology/BlockDetector#13 and my GitHub doesn't let me resolve them atm 🙄 ).

Not sure what's going on with all these Behaviors failures there 🤔 There was no test/moveMTE branch, but Behaviors does have MTE tests. Do we still need to migrate these @keturn ?

keturn · 2022-06-07T21:43:22Z

I didn't do anything with Behaviors or any of the Pathfinding modules as I'd lost track of the state of how much which ones are working or things we're keeping.

jdrueckert · 2022-06-07T21:47:14Z

I didn't do anything with Behaviors or any of the Pathfinding modules as I'd lost track of the state of how much which ones are working or things we're keeping.

Gotcha, makes sense^^
Behaviors is to be kept and IIRC there's a few tests still failing but they have been disabled for the time being, so all integration tests should work (?). Let me have a quick look.

jdrueckert · 2022-06-07T22:45:48Z

Added Terasology/Behaviors#107

keturn · 2022-06-08T00:41:29Z

RailsTest passes here 🚂

...ava/org/terasology/engine/integrationenvironment/MTEExtensionTestWithPerMethodLifecycle.java

skaldarnar · 2022-06-11T10:47:42Z

...ava/org/terasology/engine/integrationenvironment/MTEExtensionTestWithPerMethodLifecycle.java

-                        component.name, seenNames.keySet()));
+        assertThat(component.eventReceived).isTrue();
+        assertThat(component.name).isNotNull();
+        assertThat(seenNames).isEmpty();


This currently fails, because seenNames is a static class member and thus carried over even on PER_METHOD runs, it seems.

skaldarnar · 2022-06-11T10:50:49Z

...ava/org/terasology/engine/integrationenvironment/MTEExtensionTestWithPerMethodLifecycle.java

+        // FIXME: what to assert here does this make sense or should we drop the whole thing
+        //     and say it's covered by LifecyclePerMethodInjectionTest?


I'm not really sure about this either - I definitely want to keep the part of the test chat checks for which entities/how many are registered. Not sure about the part below, but maybe we can keep it for now with this comment to feel less bad when eventually removing it ^^

…vironment/MTEExtensionTestWithPerMethodLifecycle.java

skaldarnar · 2022-06-11T11:06:55Z

1ff3c70 fixes the test failure in per-method tests, but I can reliably reproduce another test failure locally in a different location:

[ChunkRegionFutureTest](file:///home/skaldarnar/Code/movingblocks/ts-iota/engine-tests/build/reports/tests/integrationTest/classes/org.terasology.engine.integrationenvironment.ChunkRegionFutureTest.html). [createChunkRegionFuture(EntityManager, RelevanceSystem, MainLoop)](file:///home/skaldarnar/Code/movingblocks/ts-iota/engine-tests/build/reports/tests/integrationTest/classes/org.terasology.engine.integrationenvironment.ChunkRegionFutureTest.html#createChunkRegionFuture(EntityManager,%20RelevanceSystem,%20MainLoop))

The tests run into a timeout when waiting for the chunk region future to complete. The failure message is coming from the exception thrown in

Terasology/engine-tests/src/test/java/org/terasology/engine/integrationenvironment/ChunkRegionFutureTest.java

Line 40 in 3674bbb

mainLoop.runUntil(chunkRegionFuture.getFuture());

Edit: The test failure is also present on develop and was introduced by merging #5041 (which also had the failing test). See https://github.com/MovingBlocks/Terasology/runs/6784174504.

skaldarnar · 2022-06-11T11:18:20Z

#5043 should fix the tests, which enables merging this PR.

test(MTE): test injection with IsolatedMTEExtension

a75258a

keturn added Category: Test/QA Requests, Issues and Changes targeting tests and quality assurance Category: Crash Requests, Issues and Changes targeting unexpected terminations, segfaults, etc. labels Jun 1, 2022

keturn mentioned this pull request Jun 1, 2022

MTE creates engines more than once for some tests #5038

Closed

keturn force-pushed the test/mteEngineOverlap branch from dd2ea14 to cac1e55 Compare June 3, 2022 17:46

test(MTE): remove @IsolatedMTETest; use JUnit TestInstance Lifecycle …

a43627f

…instead

keturn force-pushed the test/mteEngineOverlap branch from cac1e55 to a43627f Compare June 3, 2022 17:51

keturn marked this pull request as ready for review June 3, 2022 18:05

keturn changed the title ~~test(MTE): test injection with IsolatedMTEExtension~~ test(MTE): replace IsolatedMTEExtension with JUnit's TestInstance.Lifecycle Jun 3, 2022

keturn mentioned this pull request Jun 3, 2022

Convert StorageManagerTest to MTE #5032

Closed

6 tasks

keturn added 3 commits June 4, 2022 11:16

Merge remote-tracking branch 'origin/develop' into test/mteEngineOverlap

443fce0

# Conflicts: # engine-tests/src/main/java/org/terasology/engine/integrationenvironment/jupiter/MTEExtension.java

Merge remote-tracking branch 'origin/develop' into test/mteEngineOverlap

0629379

engine-tests/src/main/java/org/terasology/engine/integrationenvironment/Engines.java engine-tests/src/main/java/org/terasology/engine/integrationenvironment/jupiter/MTEExtension.java

Merge branch 'develop' into test/mteEngineOverlap

1b230c2

skaldarnar reviewed Jun 7, 2022

View reviewed changes

test: doc corrections

4a71e30

Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>

Merge branch 'develop' into test/mteEngineOverlap

77ad12b

keturn added 2 commits June 8, 2022 10:52

test: warn when Truth.assertThat is not followed by assertion

87d4fa9

test: fix incorrect parentheses on assertThat isEmpty

64a0291

skaldarnar requested changes Jun 11, 2022

View reviewed changes

Update engine-tests/src/test/java/org/terasology/engine/integrationen…

1ff3c70

…vironment/MTEExtensionTestWithPerMethodLifecycle.java

skaldarnar and others added 2 commits June 11, 2022 13:19

chore: fix checkstyle warning on declaration order

97eb372

Merge branch 'develop' into test/mteEngineOverlap

cd30f6e

skaldarnar approved these changes Jun 11, 2022

View reviewed changes

skaldarnar merged commit 7dbe872 into develop Jun 11, 2022

skaldarnar deleted the test/mteEngineOverlap branch June 11, 2022 12:13

keturn mentioned this pull request Jun 12, 2022

engine lifecycle / test case isolation Terasology/ModuleTestingEnvironment#75

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(MTE): replace IsolatedMTEExtension with JUnit's TestInstance.Lifecycle #5039

test(MTE): replace IsolatedMTEExtension with JUnit's TestInstance.Lifecycle #5039

keturn commented Jun 1, 2022 •

edited by jdrueckert

Loading

keturn commented Jun 2, 2022 •

edited

Loading

keturn commented Jun 2, 2022

keturn commented Jun 6, 2022

skaldarnar Jun 7, 2022

skaldarnar Jun 7, 2022

keturn Jun 7, 2022

keturn Jun 7, 2022

skaldarnar commented Jun 7, 2022

jdrueckert commented Jun 7, 2022

keturn commented Jun 7, 2022

jdrueckert commented Jun 7, 2022

jdrueckert commented Jun 7, 2022

keturn commented Jun 8, 2022

skaldarnar Jun 11, 2022

skaldarnar Jun 11, 2022

skaldarnar commented Jun 11, 2022 •

edited

Loading

skaldarnar commented Jun 11, 2022

	// There should be one entity, created by the @BeforeAll method
	// There should be one entity, created by the @BeforeEach method for this one test

		// FIXME: what to assert here does this make sense or should we drop the whole thing
		// and say it's covered by LifecyclePerMethodInjectionTest?

test(MTE): replace IsolatedMTEExtension with JUnit's TestInstance.Lifecycle #5039

test(MTE): replace IsolatedMTEExtension with JUnit's TestInstance.Lifecycle #5039

Conversation

keturn commented Jun 1, 2022 • edited by jdrueckert Loading

Related

How to test

keturn commented Jun 2, 2022 • edited Loading

keturn commented Jun 2, 2022

keturn commented Jun 6, 2022

skaldarnar Jun 7, 2022

Choose a reason for hiding this comment

skaldarnar Jun 7, 2022

Choose a reason for hiding this comment

keturn Jun 7, 2022

Choose a reason for hiding this comment

keturn Jun 7, 2022

Choose a reason for hiding this comment

skaldarnar commented Jun 7, 2022

jdrueckert commented Jun 7, 2022

keturn commented Jun 7, 2022

jdrueckert commented Jun 7, 2022

jdrueckert commented Jun 7, 2022

keturn commented Jun 8, 2022

skaldarnar Jun 11, 2022

Choose a reason for hiding this comment

skaldarnar Jun 11, 2022

Choose a reason for hiding this comment

skaldarnar commented Jun 11, 2022 • edited Loading

skaldarnar commented Jun 11, 2022

keturn commented Jun 1, 2022 •

edited by jdrueckert

Loading

keturn commented Jun 2, 2022 •

edited

Loading

skaldarnar commented Jun 11, 2022 •

edited

Loading