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.

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

VertexBatchTest.Locking failing locally #719

Closed
Barsonax opened this issue Apr 20, 2019 · 8 comments
Closed

VertexBatchTest.Locking failing locally #719

Barsonax opened this issue Apr 20, 2019 · 8 comments
Labels
Bug It's broken and should be fixed Core Area: Duality runtime or launcher Help Wanted Contributions especially appreciated Unit Tests Good candidate for adding more tests
Milestone

Comments

@Barsonax
Copy link
Member

Barsonax commented Apr 20, 2019

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

  • Run the test
  • It fails (well or not because it does not fail on the CI)

Workaround

  • Ignore it as it only happens locally

Analysis

  • Likely happens due to something environment specific. The question is what?
@ilexp ilexp added this to the General milestone Apr 24, 2019
@ilexp ilexp added Bug It's broken and should be fixed Core Area: Duality runtime or launcher Unit Tests Good candidate for adding more tests labels Apr 24, 2019
@ilexp
Copy link
Member

ilexp commented Apr 27, 2019

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?

@ilexp ilexp added the Help Wanted Contributions especially appreciated label Apr 27, 2019
@Barsonax
Copy link
Member Author

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
Copy link
Member

ilexp commented Apr 27, 2019

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
Copy link
Member

ilexp commented Apr 28, 2019

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

@Barsonax
Copy link
Member Author

Will check when iam at my pc

@Barsonax
Copy link
Member Author

It did not fix the issue

@Barsonax
Copy link
Member Author

Barsonax commented Mar 1, 2020

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 added a commit that referenced this issue Apr 26, 2020
* Added a utlity method to properly test if memory is released

* added xml docs

* moved and renamed garbagecollectiontestset

* fix missing include
@Barsonax
Copy link
Member Author

Fixed in #719

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug It's broken and should be fixed Core Area: Duality runtime or launcher Help Wanted Contributions especially appreciated Unit Tests Good candidate for adding more tests
Development

No branches or pull requests

2 participants