[PATCH] MVTTileTest is broken

[marcello@…]

There is a race condition in test/unit/org/openstreetmap/josm/data/imagery/vectortile/mapbox/MVTTileTest.java. The test is waiting on layers, but the required MVTTile.CLEAR_LOADED image is set after the layers.

[marcello@…]

patch

[taylor.smock]

I've never seen this. Are you certain that this happens? I assume you got a stack trace.

Realistically, it should not be a problem, given that the JVM will initialize static variables when the class is loaded, not when a new object of that class is being initialized. If it is (somehow) a problem, we can just move the declaration of CLEAR_LOADED to the top of the file. But I think the JVM would not be behaving according to the specification.

As an example:

public class Test {
  public static void main(String... argv) {
    System.out.println(new EmbeddedFields().actual);
  }

  static final class EmbeddedFields {
    final boolean actual = ENABLED;
    static final boolean ENABLED = false;
  }
}

Assuming you put it in a Test.java class, java Test.java will output false.

[marcello@…]

The problem is not the static initialization.

The problem is the wait on the wrong condition in MVTTileTest.java:57. When the layers are set in MVTTile.java:55 the test thread awakens and has only 2 lines to go until the assert that fails while the runner that is supposed to set this.image has way more to do before it finally sets this.image in MVTTile.java:76. So the test thread will win and fail.

The test fails reliably on my machine when run concurrently, and succeeds when run alone. Not sure why.

P.S. I moved the initialization so that tileSource and loader are initialized once instead of 4 times.

[taylor.smock]

The problem is not the static initialization.

Sorry. I misunderstood what you were trying to say.

The test fails reliably on my machine when run concurrently

How are you running it concurrently? I've used IntelliJ's Fork Mode: method in the past, and I haven't been able to get it to fail. I can see how it could though (maybe machine dependent?)

P.S. I moved the initialization so that tileSource and loader are initialized once instead of 4 times.

I'd rather not. You could convince me with MapboxVectorCachedTileLoader since it doesn't have mutable state (in and of itself -- some of its fields, like the cache, do have mutable state).

Anyway, I did some quick profiling using IDEA, and the setup method didn't appear in CPU usage, and, while it did appear in memory allocations, it was 2.37 MB (<2% of all). All of those allocations are from the cache.

[taylor.smock]

In 18892/josm:

Fix #23257: MVTTileTest has an infrequently hit race condition (patch by marcello, modified)

The race condition is dependent upon calculation speed; if the layers get loaded
prior to the image loading, it is possible for us to get to the point where
we expect the image to be loaded, but it isn't yet.

Modifications are as follows:

• Reinitialize tileSource and loader before each test, but initialize the cache once and clear that between tests.

[marcello@…]

I'm running the tests through gradle, in parallel mode:

<===========--> 91% EXECUTING [13s]
> :test > 8 tests completed
> :test > Executing test org.openstreetmap...tagging.presets.TaggingPresetReaderTest
> :test > Executing test org.openstreetmap.josm.actions.AddImageryLayerActionTest
> :test > Executing test org.openstreetmap.josm.actions.CopyActionTest
> :test > Executing test org.openstreetmap.josm.actions.AboutActionTest
> :test > Executing test org.openstreetmap.josm.actions.AlignInLineActionTest

On my machine (4 cores) it forks 5 VMs for test execution and then schedules the test classes more or less at random. All 3000+ unit tests take ~4 minutes.

---

Yes, but in real life you will be using the same cache for all tiles and not a new cache for every new tile.

[taylor.smock]

On my machine (4 cores) it forks 5 VMs for test execution and then schedules the test classes more or less at random. All 3000+ unit tests take ~4 minutes.

I've made some improvements to the test code so that running the tests sequentially takes ~8 minutes (3300 tests on a i7-7920HQ@3.1 GHz). This includes a class that you might not be running (MapCSSStyleSourceFilterTest, ~42s).

Of particular note, ProjectionRegressionTest is one of the "worst" standard tests -- it takes ~41s.

You might be able to speed the tests up with test.without.gc=true (system property, see JosmDefaults for what it does). I think it can be the default, but it isn't (mostly for the tests run on CI).