AdamsLair / duality

a 2D Game Development Framework
https://adamslair.github.io/duality
MIT License
1.41k stars 288 forks source link

VertexBatchTest.Locking failing locally #719

Closed Barsonax closed 4 years ago

Barsonax commented 5 years ago

Summary

The VertexBatchTest.Locking is failing for me locally

Expected: False
  But was:  True

   at Duality.Tests.Drawing.VertexBatchTest.Locking() in C:\Git\duality\Test\Core\Drawing\VertexBatchTest.cs:line 90

How to reproduce

Workaround

Analysis

ilexp commented 5 years ago

The specific part of the test that is failing is this:

// Make sure that our locks released properly, i.e. allowing the array to be garbage collected
WeakReference weakRefToLockedData = new WeakReference(typedBatch.Vertices.Data);
Assert.IsTrue(weakRefToLockedData.IsAlive);
typedBatch = null;
abstractBatch = null;
GC.Collect();
Assert.IsFalse(weakRefToLockedData.IsAlive);

Which is dependent on garbage collection behavior, or more specifically the assumption that GC.Collect will free the vertex data array synchronously before returning. Could this be related? Are there ways to ensure this across runtime / GC versions?

Barsonax commented 5 years ago

It does not do it synchronously: https://stackoverflow.com/questions/748777/run-gc-collect-synchronously

Also I think depending on the GC like this is very error prone as it can change between runtime or config settings.

ilexp commented 5 years ago

Also I think depending on the GC like this is very error prone as it can change between runtime or config settings.

It is, but since this part of the test is essentially a GC test, I don't really see a way around relying on GC behavior in some way. The current implementation is flaky though.

It does not do it synchronously: https://stackoverflow.com/questions/748777/run-gc-collect-synchronously

That's probably the issue then. Could be worth a try to use this overload instead and also wait for pending finalizers afterwards.

ilexp commented 5 years ago

Adjusted the test as described above. @Barsonax Can you check whether the issue is fixed on your machine?

Barsonax commented 5 years ago

Will check when iam at my pc

Barsonax commented 5 years ago

It did not fix the issue

Barsonax commented 4 years ago

Only happens in Debug mode, under release mode the test runs succesfully. Debug seems to be keeping instances alive for longer than needed which makes sense as its handy for debugging.

So this error can be fixed by skipping this test in debug mode (no sense in running it if it always fails). In nunit this can be done by making a custom NUnitAttribute and implementing IApplyToTest. Ofcourse a reasonable message should be given explaining that it does not run in debug.

Barsonax commented 4 years ago

Fixed in #719