Avoid expectFail in the test suite

[fendor]

expectFail interprets a test failure for any reason as a success. But since we don't check why exactly the test is failing, we lose valuable information.
Thus, many of the tests that use expectFail are likely not really testing what they are supposed to be.

In my opinion, we should replace all occurrences of expectFail with an assertion that shows what is failing precisely, or delete/ignore the respective test case.

[sgillespie]

Most of the expectFail usages are to "disable" tests known to be broken. Is this a valid use, or should they all be updated/removed?

[fendor]

I think expectFail is fine to temporarily disable certain tests on certain platforms.
The issue is with unconditionally disabling a test with expectFail, as we lose over time the information why the test is supposed to be failing. In these situation, as long as it isn't too flaky, we should have an assertion that documents the current behviour, with a comment explaining what is going wrong!

[sgillespie]

I updated a few tests in #4402 for discussion. Is this basically what you were imagining?

[fendor]

Yes, that's basically what I am imagining, thanks!
I think it would be nice to also document what the intended behaviour would be, preferably in the code. E.g., adding a hoverTestExpectedFail which takes the expected but incorrect value, and also the value we would like to have as input parameters.

Do you think that would make sense?

[sgillespie]

I like the idea of correctly typing both possibilities. Say,

hoverTestExpectFail :: TestName -> Position -> T.Text -> T.Text -> TestTree
hoverTestExpectFail name pos _ideal expected = hoverTest name pos expected

EDIT: My other thought is to use something at the type level to make it a bit easier to read, eg:

, hoverTestExpectFail'
        "import"
        (Position 2 18)
        (IdealBehavior "Control.Monad**")  -- Ensure no extra newlines
        (CurrentBehavior "Control.Monad\n\n")

-- <-- Snip -->
hoverTestExpectFail' :: TestName -> Position -> ExpectBroken BrokenIdeal -> ExpectBroken BrokenCurrent -> TestTree
hoverTestExpectFail' name pos _ expected = hoverTest name pos (unCurrent expected)

[fendor]

This looks nice to me!

[sgillespie]

This looks nice to me!

Which one?

[fendor]

Both look good to me, the second proposal sounds better, and I'd be curious what the tests look like in practice with this change.

[sgillespie]

I've updated most of the tests. I'm not really sure what to do with the golden tests, so I left those alone. Otherwise, they all make use of a simple GADT that's meant to distinguish between current and ideal behaviors.

[fendor]

Thanks, closed by #4402