discussion: buildModel and CompilationUnits order

[surli]

While working on #1928 I get stuck on a bug which was appearing when building the model from the input path ./src/test/java/spoon/test/ and disappearing when building the model from path: ./src/test/java/spoon/test/annotation/testclasses/shadow.
And my entire test was built only with types coming from ./src/test/java/spoon/test/annotation/testclasses/shadow.
The bug was in fact coming because one type was resolving before another one I needed in the first case, and after it in the second case.

So in other word, I got a bug because of the order of resolution of the compilation units. The interesting point is that I was quite confident on my implementation, until my test crashes because I changed the path and so changed the order of resolution.

What action should we take about this?

1. None
2. To fix the order in which compilation units are sorted in JDTBasedSpoonCompiler and to rely only on this order
3. To define a random order with a seed, and to check in CI tests with different seeds, to ensure everything's passing even if the classes are not resolved in the same order

WDYT?

[monperrus]

I would go for 2), because 3) will be super hard to debug for us, especially when unknown people will come with a bug report.

[surli]

2), because 3) will be super hard to debug for us, especially when unknown people will come with a bug report.

Don't agree: for me in 3, the order is defined internally in Spoon by a default-coded seed. So, it's the same as in 2. But we also allow for test-purpose to change the seed and check the behaviour remains: i.e. the resolution of the model is independent from the resolution of the CUs.

[pvojtechovsky]

The bug was in fact coming because one type was resolving before another one I needed in the first case, and after it in the second case.
So in other word, I got a bug because of the order of resolution of the compilation units.

I do not understand. "resoultion?" Do you speak about compilation or about conversion of java classes by reflection into shadow CtClass? Or anything else?

I tried to reproduce this problem, but it doesn't fail in AnnotationTest#testAnnotationArray in my installation...

[surli]

I do not understand. "resoultion?"

When you build the model, you first send the sources to JDT which creates and resolve the CompilationUnits then those CompilationUnits are visited to create Spoon model.
And in JDTBasedSpoonCompiler we take an array of CompilationUnits: we do not guarantee any order in this array, it's completely dependent of JDT.

I tried to reproduce this problem, but it doesn't fail in AnnotationTest#testAnnotationArray in my installation...

That's actually my point: it's not failing certainly because on your computer CompilationUnits are not resolved on the same order than on mine.

[pvojtechovsky]

Do you know what exactly depends on that order? Or in other words... why the order is relevant at all?

[surli]

why the order is relevant at all?

In my case the tests are failing because of changes done in CtAnnotationImpl, which depends on the order. I don't think we should depends on the order actually. But we should test that we are not depending on the order: that's why I propose to fix the order with a seed and to be able to change the seed only inside some tests to ensure that.

[pvojtechovsky]

I don't think we should depends on the order actually.

Super, I wanted to hear that.

But we should test that we are not depending on the order

it is interesting idea. If you know how to do it, and it will not cost 3 times longer build time, then I welcome that ;-)

@monperrus WDYT?

[surli]

and it will not cost 3 times longer build time

My idea is to only check that on Jenkins, not on Travis, to avoid increasing the response time on PR/commits.