cabal check testuite: add sanity checks

[ffaf1]

cabal check testsuite: sanity checks (i.e.: this bit).

Missing:

• “no internal unnamed library” (see "Found one or more unnamed internal libraries. " check in cabal check is cruft #8247)
• unsupported benchmark/unsupported testsuite (cabal correctly chokes on parsing)
• missing main from testsuite/benchmark (see Wrong parse error when main-is missing from test-suite #8246)

---

Please include the following checklist in your PR:

• Patches conform to the coding conventions.
• Any changes that could be relevant to users have been recorded in the changelog.
• The documentation has been updated, if necessary.

Please also shortly describe how you tested your change. Bonus points for added tests!

[jneira]

Amazing work, to improve the test suite and coverage is one of the most valuables contributions to any codebase and cabal of course. Many thanks.

[ulysses4ever]

I wonder if hardcoding error messages in the test suite is really necessary, or a more flexible check (pun intended) that there is a warning (or that exit code is non zero) would suffice. The latter would be easier to maintain probably.

[ffaf1]

The question is sound, thanks for bringing it up.

If you test on the mere presence of errors/warnings (without comparing error messages):

• on one hand, you risk marring some tests. As an example:
  

  cabal/Cabal/src/Distribution/PackageDescription/Check.hs

  Lines 743 to 751 in fd6de6b

  	, check (maybe False isAbsoluteOnAnyPlatform (repoSubdir repo)) $
  	PackageDistInexcusable
  	"The 'subdir' field of a source-repository must be a relative path."
  	, do
  	subdir <- repoSubdir repo
  	err <- isGoodRelativeDirectoryPath subdir
  	return $ PackageDistInexcusable $
  	"The 'subdir' field of a source-repository is not a good relative path: " ++ show err

  
  “relative path” check and “good relative path check” are not the same; it is important for the end user to get a different specific message. By testing on warning we lose the assurance that the correct message is being printed (and not — e.g. — the other, both, multiple of the same type, a third unrelated one).
• On the other hand it is way more easier to maintain; slight changes in output will not break tests. The more “internal” the change is, the bigger the breakage: if we were to change Warning: — which now starts every warning message — with Attention:, 95% of the testsuite would instantly turn red.

I wrote tests checking against output:

• following what was already there in the repo; and
• after reading this bit in the testsuite README:
  

  cabal/cabal-testsuite/README.md

  Lines 123 to 127 in fd6de6b

  	**How do I turn off accept tests? My test output wobbles too much.**
  	Use `recordMode DoNotRecord`. This should be a last resort; consider
  	modifying Cabal so that the output is stable. If you must do this, make
  	sure you add extra, manual tests to ensure the output looks like what
  	you expect.

  
  The one I wrote are (mostly) not accept-tests, but the bit on output stability I feel still applies.

The question is even more sound considering how I am approaching writing tests for cabal check. Along this pull request (22 tests), there is #8250 (another 15) and the one I am writing locally (+15). I am painstakingly going through all PackageDescription/Check.hs; I expect the final testsuite for cabal check to have several hundred tests.

Tests are indeed not cheap to maintain, any kind of direction on what maintainers are happy with is welcome.

CC @andreasabel who mentors me and is for sure interested.
I am happy with whatever decision is taken, reimplementing cabal check goes on!

[jneira]

well I think messages don't change so frequently and fix the test in front of no semantic changes is almost trivial: you get the test error and change the message
you also check the correct message is presented in the appropriate state

there are not unit tests for Chech messages, I want to remember there were some but not sure. Maybe the correct balance between unit and integration tests could make msintanence more lightweight

[ffaf1]

@ulysses4ever a consideration on tests maintainability. My plan to reimplement cabal check in #8211 includes this step: “tipify cabal check errors (as now they are Strings)”.

Once that is done there should be a way to convert the testsuite from checking against Strings to checking against data constructors, much more robust. Some of the switch could probably also be automated, at least for simple tests (e.g. this one, with just three files and a simple cabal.test.hs body).

Current testsuite would still be useful in refactoring cabal check errors; moreover the refactoring should give us some data on how the stability of cabal check output and how painful it is to rewrite a test (I had to rewrite the output of a test in #8258, I just passed --accept to cabal-test, that one felt easy).

I cannot promise a timeframe because my plate is quite full, but ideally I think checking against constructors will give us maintainability and precision at the same time.

[ulysses4ever]

@ffaf1 sounds good. Being able to check type constructors sounds exciting! But I understand everyone is busy and don't hold my breath.

@jneira

I think messages don't change so frequently and fix the test in front of no semantic changes is almost trivial

That is true. I thought about cases when you have to change many/all tests in bulk because e.g. the format of messages changed. Or when you have the same message in many tests. But maybe this is unlikely to happen.

[ulysses4ever]

I'm not sure whether it's best to merge 22 commits individually or squash them. It seems they have been merged individually. I think I'd prefer squashing (in the future, at least).

[ffaf1]

Ouch, I will do that next PR!

[Mikolaj]

It's in a clearly separate loop of the git history, so not much harm (visible, e.g., with gitk).