Fix flaky boot def test

[eddiemundo]

Should fix #2677

I could not reproduce it locally, but I found in the logs that it is possible for getHover to eat a ghcide/reference/ready custom message. If that happens the skipManyTill ghcide/references/ready will be starving until timeout. Not sure why this happens more often (or only?) on the 9.0.2 ubuntu-latest tests.

The fix skips messages until the hover response or ghcide/reference/ready, and depending on which was received skips messages until receiving the other.

In all the other places I could find where something similar happens they either do getHover or skipManyTill ghcide/references/ready but not both, and so that's why we don't see these problems in other tests.

As a side note at first I didn't realize more than one ghcide process was being run so the logs were very confusing.

[pepeiborra]

Should fix #2677

I could not reproduce it locally, but I found in the logs that it is possible for getHover to eat a ghcide/reference/ready custom message. If that happens the skipManyTill ghcide/references/ready will be starving until timeout. Not sure why this happens more often (or only?) on the 9.0.2 ubuntu-latest tests.

Good catch, getHover (and all other lsp-test request-response primitives) definitely eat messages.

Re 9.0.2, I have seen this test fail in the 9.2.1 branch as well, so it's just timing issues.

[pepeiborra]

Thank you!!!

[michaelpj]

Can we get a comment in the code? It's a bit non-obvious what's going on there for future people.

Is there something systematic we should be doing to make this happen less often? Should we alter getHover & friends somehow?

[eddiemundo]

Can we get a comment in the code? It's a bit non-obvious what's going on there for future people.

Yeah, I'll add a comment.

Is there something systematic we should be doing to make this happen less often? Should we alter getHover & friends somehow?

The getX then skipUntilY pattern has caused some flaky tests in the past (and this one) because I guess we aren't fully aware of the possible orders messages can go to the client.

Something that might be better in some cases is a waitForMsgSet that can return the set of messages in the order specified, but where the order of the set doesn't matter.

So maybe

waitForMsgSet :: NonEmptyList (FromServerMessage -> Bool) -> Session (NonEmptyList FromServerMessage)

and a bunch of predicates defined for common types like the hover response, definition response etc. The idea is that it will wait for all of these predicates to be true in any order, and return messages corresponding to each predicate in the same order as the predicates were listed.

Not sure if this is idiomatic regarding the parser type, maybe there is a higher level of doing the same thing, but I think something like this would be convenient.

Then after maybe the getX should be banned... but then that would also mean the pattern would be sendX params then waitForMsgSet (NonEmpty.singleton X) which is less convenient.

[michaelpj]

I don't really know enough about how the tests are written to say, but I encourage you to keep thinking about it!