Code completion order arbitrary

[dcharkes]

The order of class and constructor definitions is undefined:

https://dart-review.googlesource.com/c/sdk/+/283341/2/pkg/analysis_server/test/services/completion/dart/location/relational_pattern_test.dart#113

I'm getting class/constructor reorderings on an unrelated CL.

cc @bwilkerson @scheglov

[bwilkerson]

I'm surprised. I would have expected the order to be stable. We certainly haven't seen any flakiness on the bots.

Can you point me to the unrelated CL? I'll probably need to step through the debugger to understand where the change in the order is occurring.

[dcharkes]

https://dart-review.googlesource.com/c/sdk/+/289223/6/pkg/analysis_server/test/services/completion/dart/location/relational_pattern_test.dart

[dcharkes]

Before my change:

  A01
    kind: constructorInvocation
  A01
    kind: class
  A02
    kind: constructorInvocation
  A02
    kind: class
  B01
    kind: class
  B01
    kind: constructorInvocation

Why does B01 have the class kind first, and the others have the constructorInvocation kind first?

[bwilkerson]

It isn't a very satisfying answer, but the truth it that the completion support was organically grown, so some of the code is duplicated in ways that introduce inconsistencies like that. It's something I hope to be able to address next quarter.

[scheglov]

I think we probably cannot expect that the server returns completion suggestions in any particular order.
But we can expect that it has relevance set, so that the client can sort it.

We try to sort suggestions in our tests, specifically to prevent such random order changes.
By default we sort by relevance, and then by the completion string.
Unfortunately, these are probably the same in this case.
The solution is probably to sort also by kind.

[bwilkerson]

I think we probably cannot expect that the server returns completion suggestions in any particular order.

Why? Is there some non-determinism that I'm forgetting about?

The solution is probably to sort also by kind.

That seems reasonable to me, at least for these tests. The purpose of these tests is not to test the order of the suggestions, just the presence or absence.

But I'd suggest that we stop sorting by relevance. I don't want to have to re-write the expectations every time the relevance changes. We have separate tests for relevance and shouldn't be testing that here, especially since the fact that it's being tested for is hidden.

[scheglov]

https://dart-review.googlesource.com/c/sdk/+/292640