Remove duplication in testcode

[fendor]

Extracts ghcide-tests s.t. we can add hls-test-utils as a dependency of ghcide-tests.

Did not work before with this error

Resolving dependencies...
Error: cabal: Could not resolve dependencies:
[__0] trying: ghcide-1.8.0.0 (user goal)
[__1] trying: hls-test-utils-1.4.0.0 (user goal)
[__2] trying: hls-test-utils:-pedantic
[__3] trying: temporary-1.3 (dependency of hls-test-utils)
[__4] trying: temporary:!test
[__5] trying: tasty-rerun-1.1.18 (dependency of hls-test-utils)
[__6] trying: tagged-0.8.6.1 (dependency of tasty-rerun)
[__7] trying: tagged:+deepseq
[__8] trying: tagged:+transformers
[__9] trying: template-haskell-2.18.0.0/installed-2.18.0.0 (dependency of
tagged)
[_10] trying: pretty-1.1.3.6/installed-1.1.3.6 (dependency of
template-haskell)
[_11] trying: ghc-prim-0.8.0/installed-0.8.0 (dependency of template-haskell)
[_12] trying: rts-1.0.2/installedrts (dependency of ghc-prim)
[_13] trying: split-0.2.3.5 (dependency of tasty-rerun)
[_14] trying: split:!test
[_15] trying: tasty-hunit-0.10.0.3 (dependency of hls-test-utils)
[_16] trying: call-stack-0.4.0 (dependency of tasty-hunit)
[_17] trying: call-stack:!test
[_18] trying: tasty-golden-2.3.5 (dependency of hls-test-utils)
[_19] trying: tasty-golden:!test
[_20] trying: tasty-golden:-build-example
[_21] trying: typed-process-0.2.10.1 (dependency of tasty-golden)
[_22] trying: typed-process:!test
[_23] trying: process-1.6.13.2/installed-1.6.13.2 (dependency of
typed-process)
[_24] trying: tasty-expected-failure-0.12.3 (dependency of hls-test-utils)
[_25] trying: tasty-expected-failure:!test
[_26] trying: unbounded-delays-0.1.1.1 (dependency of tasty-expected-failure)
[_27] trying: tasty-1.4.2.3 (dependency of hls-test-utils)
[_28] trying: tasty:+clock
[_29] trying: clock-0.8.3 (dependency of tasty +clock)
[_30] trying: clock:!bench
[_31] trying: clock:!test
[_32] trying: clock:-llvm
[_33] trying: tasty:+unix
[_34] trying: wcwidth-0.0.2 (dependency of tasty)
[_35] trying: wcwidth:-cli
[_36] trying: wcwidth:+split-base
[_37] trying: ansi-terminal-0.11.3 (dependency of tasty)
[_38] trying: ansi-terminal:-example
[_39] trying: colour-2.3.6 (dependency of ansi-terminal)
[_40] trying: colour:!test
[_41] trying: lsp-test-0.14.1.0 (dependency of hls-test-utils)
[_42] trying: lsp-test:!bench
[_43] trying: lsp-test:!test
[_44] trying: some-1.0.4.1 (dependency of lsp-test)
[_45] trying: some:!test
[_46] trying: some:+newtype-unsafe
[_47] trying: parser-combinators-1.3.0 (dependency of lsp-test)
[_48] trying: parser-combinators:-dev
[_49] trying: conduit-parse-0.2.1.1 (dependency of lsp-test)
[_50] trying: conduit-parse:!test
[_51] trying: safe-0.3.19 (dependency of conduit-parse)
[_52] trying: safe:!test
[_53] trying: parsers-0.12.11 (dependency of conduit-parse)
[_54] trying: parsers:!test
[_55] trying: parsers:+binary
[_56] trying: parsers:+parsec
[_57] trying: parsec-3.1.15.0/installed-3.1.15.0 (dependency of parsers
+parsec)
[_58] trying: parsers:+attoparsec
[_59] trying: attoparsec-0.14.4 (dependency of parsers +attoparsec)
[_60] trying: attoparsec:!bench
[_61] trying: attoparsec:!test
[_62] trying: attoparsec:-developer
[_63] trying: scientific-0.3.7.0 (dependency of parsers)
[_64] trying: scientific:!bench
[_65] trying: scientific:!test
[_66] trying: scientific:-bytestring-builder
[_67] trying: scientific:-integer-simple
[_68] trying: primitive-0.7.4.0 (dependency of scientific)
[_69] trying: primitive:!bench
[_70] trying: primitive:!test
[_71] trying: integer-logarithms-1.0.3.1 (dependency of scientific)
[_72] trying: integer-logarithms:!test
[_73] trying: integer-logarithms:+integer-gmp
[_74] trying: integer-logarithms:-check-bounds
[_75] trying: ghc-bignum-1.2/installed-1.2 (dependency of integer-logarithms)
[_76] trying: charset-0.3.9 (dependency of parsers)
[_77] trying: base-orphans-0.8.7 (dependency of parsers)
[_78] trying: base-orphans:!test
[_79] trying: conduit-parse:-enable-hlint-test
[_80] trying: conduit-1.3.4.3 (dependency of lsp-test)
[_81] trying: conduit:!bench
[_82] trying: conduit:!test
[_83] trying: resourcet-1.2.6 (dependency of conduit)
[_84] trying: resourcet:!test
[_85] trying: mono-traversable-1.0.15.3 (dependency of conduit)
[_86] trying: mono-traversable:!bench
[_87] trying: mono-traversable:!test
[_88] trying: vector-algorithms-0.9.0.1 (dependency of mono-traversable)
[_89] trying: vector-algorithms:!bench
[_90] trying: vector-algorithms:!test
[_91] trying: vector-algorithms:-llvm
[_92] trying: vector-algorithms:+boundschecks
[_93] trying: vector-algorithms:-unsafechecks
[_94] trying: vector-algorithms:-internalchecks
[_95] trying: bitvec-1.1.3.0 (dependency of vector-algorithms)
[_96] trying: bitvec:!bench
[_97] trying: bitvec:!test
[_98] trying: bitvec:-libgmp
[_99] trying: vector-algorithms:+properties
[100] trying: vector-algorithms:+bench
[101] trying: blaze-markup-0.8.2.8 (dependency of hls-test-utils)
[102] trying: blaze-markup:!test
[103] trying: blaze-builder-0.4.2.2 (dependency of blaze-markup)
[104] trying: blaze-builder:!test
[105] rejecting: ghcide:*test (cyclic dependencies; conflict set: ghcide,
hls-test-utils)
[__1] rejecting: hls-test-utils-1.3.0.0, hls-test-utils-1.2.0.0,
hls-test-utils-1.1.0.2, hls-test-utils-1.1.0.1, hls-test-utils-1.1.0.0,
hls-test-utils-1.0.1.0, hls-test-utils-1.0.0.0 (constraint from user target
requires ==1.4.0.0)
[__1] fail (backjumping, conflict set: ghcide, hls-test-utils)
After searching the rest of the dependency tree exhaustively, these were the
goals I've had most trouble fulfilling: ghcide, hls-test-utils

So, we extract the package and reduce some of the code duplication.

[michaelpj]

Would this also unblock #2979 ?

[fendor]

Not yet, but might be helpful to further simplify the dependency graph.

I think, we might want to design the dependency graph we want and then try to work towards it.

[pepeiborra]

I don't want to lose cabal test ghcide, can we discuss an alternative solution? Perhaps move hls-test-utils to the ghcide package?

[fendor]

can we discuss an alternative solution?

Sure, I will create a component diagram of the status quo, ideally in something that we can edit as we improve things, so that we can decide on a better course of action.

[fendor]

I created some graphs, but they aren't as helpful as hoped.

We can merge hls-test-utils with ghcide-test-utils and merge it into ghcide as sub-libraries.
I think it will feel weird if we have hls-test-utils as part of ghcide, feels like some lines are blurring. Dependency footprint of ghcide increases in that case, blaze-markup is a new dependency for ghcide afaict
However, this will further decrease the usability of stack on the hls codebase, since stack has some issues with internal sub-libraries.

[pepeiborra]

We can merge hls-test-utils with ghcide-test-utils and merge it into ghcide as sub-libraries. I think it will feel weird if we have hls-test-utils as part of ghcide, feels like some lines are blurring. Dependency footprint of ghcide increases in that case, blaze-markup is a new dependency for ghcide afaict

Duplication is not the worst code smell, in some cases it's well justified. I wonder if that's the case here. If the solution is worse than the problem, is it worth fixing it at all?

However, this will further decrease the usability of stack on the hls codebase, since stack has some issues with internal sub-libraries.

ghcide-test-utils addresses this by having a standalone cabal descriptor for the internal sub library, seems the same workaround could be applied here. What's the problem with doing this?

[fendor]

If the solution is worse than the problem, is it worth fixing it at all?

Maintenance costs are currently a real thing in this stage of HLS's life, code duplication, especially unnecessary, increases maintenance burden, slightly, in my opinion. In this case, the main benefit is a bit of untangling of dependencies.

What is so bad about not having cabal test ghcide? Personally, at least, I prefer explicit component naming, e.g. cabal test ghcide-tests is my usual invocation.

ghcide-test-utils addresses this by having a standalone cabal descriptor for the internal sub library,

ghcide-test-utils is a weird hack, it is a library that hls and others can depend on, but its modules are also included by ghcide-tests via other-modules and hs-source-dirs. So, ghcide does not depend directly on ghcide-test-utils the package, but includes its modules. I do not want to apply this same hack for hls-test-utils.

This PR actually cleans this mess up, assuming you'd agree that this is messy.

[pepeiborra]

What is so bad about not having cabal test ghcide? Personally, at least, I prefer explicit component naming, e.g. cabal test ghcide-tests is my usual invocation.

It's another thing to learn and remember when contributing to HLS. Most consumers, whether it's users or CI systems (think Nix), expect cabal test ghcide to do something sensible.

ghcide-test-utils is a weird hack, it is a library that hls and others can depend on, but its modules are also included by ghcide-tests via other-modules and hs-source-dirs. So, ghcide does not depend directly on ghcide-test-utils the package, but includes its modules. I do not want to apply this same hack for hls-test-utils.

The extra entries in other-modules and hs-source-dirs seem like a small price to pay.
Rather than replicating the hack, can we simply merge ghcide-test-utils and his-test-utils?

[fendor]

Sure, I will move the code, and we will take a look on how it feels.

EDIT: I've had a second idea. Since the only thing ghcide-tests take from hls-test-utils are the ignoreInEnv functions (e.g. ignore this test-case if these conditions hold), we could extract a small library and make ghcide-tests and hls-test-utils depend on that library. A library tasty-ignore-env or something like that
Is that better or worse?

Advantage:

• No code duplication
• Other libs such as hie-bios could depend on it as well since we do ignore in some environment test-cases

Disadvantage:

• Another library to maintain.

[pepeiborra]

Is that better or worse?

Much beter

[fendor]

Last question, maintenance of this new package in the hls repository or should I just create a package and we maintain it in another repository. I am inclined to do the latter to not increase the CI load for this repository. That or we plan to increase our CI capabilities.

[pepeiborra]

I am in favour of sticking to a monorepo for as long as possible. What impact do you expect adding this new package will have on CI?

[fendor]

Probably not a lot, but the CI is already stretched to its limits, I feel. I don't think we know how much caching space we need per GHC version, and we only have 10GB for all supported GHC versions, adding more libs will just increase the size.
Maybe maerwalds work on utilising cabal-cache together with an amazon s3 bucket will help in the future.

In other words, it is unlikely to be noticeably worse, but the CI seems overloaded at the moment.

[fendor]

I don't intend to follow up on this in the foreseeable future. Closing it.