Sort out test suite

[Bodigrim]

Current structure of text repo is more complicated than I would like it to be. There are four separate packages: text itself, plus text-tests and th-tests, plus benchmarks.

The reason to have tests as separate packages is that they depend on test-framework, which itself depends on text. Moreover, just separating tests is not sufficient, because any change to text would cause rebuilding test-framework, test-framework-quickcheck2, test-framework-hunit, regex-posix, regex-base, xml, before being actually able to build and run the test suite. This is a very long feedback loop, so a hack is employed: instead of depending on a local package text, the test suite just lists all text modules as its own, adding ../src to hs-source-dirs.

This hack allows to avoid rebuilding intermediate packages, but has its own drawbacks such as rebuilding the same source module twice (because it is listed under two unrelated components). More importantly, this, strictly speaking, does not necessarily test text package as it is presented to users, with all its flags and ghc-options, but rather tests its source modules bundled independently, which are not warranted in sync.

Further, th-tests are a separate package, which depends on text package indeed, probably because of Template Haskell stage restrictions (?). It means that it rebuilds for ages despite all efforts put into text-tests.

Bottom line is that

• we must update and keep in sync four cabal files,
• we need to list a ton of files under extra-source-files,
• coverage report does not really work,
• running all tests is painfully slow.

This PR:

• Replaces test-framework with tasty, which is its more modern counterpart.
• Since tasty does not depend on text, a hack with hs-source-dirs: ../src can be abandoned without causing excessive rebuilds.
• Consequently, there is no reason to keep two separate test suites. They can be merged, saving linking time and bookkeeping.
• We can actually immerse our test suite into the main package. Minus two packages, simpler setup for everyone.

Areas of improvement:

• Benchmarks stopped to build with GHC 7.10 because of the choice of semigroups flags.
• It would be nice to build text with -fdeveloper on CI, but it breaks benchmarks.

I'll address these drawbacks in an upcoming PR, improving our situation with benchmarks in the same fashion as this PR sorts out tests, but I hope they are acceptable in the meantime.

[Lysxia]

Nice work!

We can actually immerse our test suite into the main package.

I'm a little worried about that. The main reason for merging seems to be that the circular dependency is gone with the move to tasty, but that seems a very brittle assumption considering how general-purpose text aims to be.

Is cabal able to resolve dependencies at the level of components within packages?

[parsonsmatt]

Further, th-tests are a separate package, which depends on text package indeed, probably because of Template Haskell stage restrictions (?). It means that it rebuilds for ages despite all efforts put into text-tests.

I don't think this is the case - the stage restrictions operate on a per-module basis, not per-package. The persistent package also separates out the Template Haskell into a separate package, and I was told that it was from a desire to not depend on template-haskell as a necessary part of the package. So that may be a similar factor here? I'm planning on merging persitent-template back into persistent at some point, so I'd be tentatively in favor of merging th stuff into a logically reasonable place.

[parsonsmatt]

i love too see it

[parsonsmatt]

Is the intent to restore this tested-with clause? Or are we dropping 7.10.3 support?

[Bodigrim]

See the bottom of #314 (comment)

This (temporarily) drops support of GHC 7.10 for benchmarks only. The text package itself still supports GHCs back to 7.0.

As @chessai pointed below, it could be a good idea to drop support of obsolete versions of GHC, but this is, strictly speaking, a separate matter, and not a decision I'm implementing in this PR.

[parsonsmatt]

This change doesn't appear to be related to a test-framework => tasty change. I'm curious why it's done

[Bodigrim]

Good question!

TA.length is defined only when the package is built in developer mode:

text/src/Data/Text/Array.hs

Lines 35 to 37 in 6d8ae4d

	#if defined(ASSERTS)
	, length
	#endif

The existing test suite lists Data.Test.Array as its own module and thus can enforce -DASSERTS irrespective to actual developer flag:

text/tests/text-tests.cabal

Lines 89 to 90 in 6d8ae4d

	cpp-options:
	-DASSERTS -DTEST_SUITE

Since now the test suite depends on a proper text package, it lost its ability to enforce -DASSERTS on its modules, and now this is guided by developer flag only. In order to compile tests when this flag is not enabled, I had to avoid TA.length and replace it with sizeOfByteArray#.

It would be nice to put constraint: text +developer into cabal.project, so that all developers (and CI) have it enabled by default. Unfortunately, such configuration breaks benchmarks. As mentioned at the bottom of #314 (comment), I'll fix this in an upcoming PR.

[chessai]

For context: All core libraries are dropping 7.x support.

…

On Wed, Mar 10, 2021, 09:52 Matt Parsons ***@***.***> wrote: ***@***.**** approved this pull request. [image: image] <https://user-images.githubusercontent.com/7310112/110657191-dd61c780-817d-11eb-8db6-089d1f93ddd3.png> i love too see it ------------------------------ In benchmarks/text-benchmarks.cabal <#314 (comment)>: > @@ -15,7 +15,7 @@ category: Text build-type: Simple tested-with: GHC==9.0.1, GHC==8.10.4, GHC==8.8.4, GHC==8.6.5, GHC==8.4.4, - GHC==8.2.2, GHC==8.0.2, GHC==7.10.3 Is the intent to restore this tested-with clause? Or are we dropping 7.10.3 support? ------------------------------ In tests/Tests/Regressions.hs <#314 (comment)>: > @@ -105,8 +109,8 @@ t227 = t301 :: IO () t301 = do assertEqual "The length of the array remains the same despite slicing" - (TA.length originalArr) - (TA.length newArr) + (I# (sizeofByteArray# (TA.aBA originalArr))) + (I# (sizeofByteArray# (TA.aBA newArr))) This change doesn't appear to be related to a test-framework => tasty change. I'm curious why it's done — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#314 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEOIX25VJS4MCIUXLLJ533DTC6BSLANCNFSM4Y42UQNA> .

[Bodigrim]

The persistent package also separates out the Template Haskell into a separate package, and I was told that it was from a desire to not depend on template-haskell as a necessary part of the package. So that may be a similar factor here?

I don't think so, text library itself already depends on template-haskell:

text/text.cabal

Line 168 in 6d8ae4d

template-haskell >= 2.5 && < 2.18

[Bodigrim]

I'm a little worried about that. The main reason for merging seems to be that the circular dependency is gone with the move to tasty, but that seems a very brittle assumption considering how general-purpose text aims to be.

@Lysxia I understand your worries, but I do not expect tasty to acquire more dependencies overnight, and we can stick to older versions if needed. In my experience having tests as a part of the main package is a nice quality-of-life improvement, and we use the same approach in bytestring and unix, which are deeper in a dependency graph than text. In the worst case scenario we can always revert to separate packages in future.

[Lysxia]

LGTM (just removing cruft and switching to tasty)

[Bodigrim]

CC @tathougies (GitHub still resists to adding you as reviewer).

Unless there are further comments, I'll merge this tomorrow.