search

FlexTradeUKLtd / jfixture

JFixture is an open source library based on the popular .NET library, AutoFixture
MIT License
105 stars 22 forks source link

Resolve sort order between ambiguous constructor and factory methods #50

Closed TWiStErRob closed 4 years ago

TWiStErRob commented 5 years ago

We have a type similar to TypeWithAmbiguousConstructors and ran into a problem of having flaky tests. This was due to Class#getMethods() and Class#getConstructors() having no guarantees on order.

getMethods explicitly states:

The elements in the returned array are not sorted and are not in any particular order.

This means that when JFixture gets those arrays and then sorts them the sort will be stable and keep the order in the input array, meaning different runs will pick different methods/ctors depending on JVM internals (class loading, potentially even class file structure, or compilation).

This resulted in a different constructor being called randomly on some tests. This also depended on processor usage (parallel JUnit tests were being run) and manifested more that way. One of the few constructors were not compatible with JFixture and threw an exception, this is why we were able to track down the root case.

TWiStErRob commented 5 years ago

Added some questions, the rest is clear and will do.

philipwhiuk commented 5 years ago

I think I'm happy with this now. Given it's been hanging around I'll give people this week to look it over and then just merge it in if there's no more feedback.

Thanks @TWiStErRob!

philipwhiuk commented 5 years ago

Hmm I'm not sure how the conflict should be resolved :(

TWiStErRob commented 5 years ago

@philipwhiuk Resolved it on the web interface (the mockito PR added Object.class, this PR removed new ArrayList; adjacent, but unrelated changes)

I recommend pressing "Squash and merge" when you merge this, or I can pre-squash in the evening if you want.

TWiStErRob commented 5 years ago

Can you please re-trigger CI? I think that's the only thing missing in this PR.