guide the user towards solving test-plan issues

[gelisam]

This simpler change (tweaking an error message) came up while discussing a larger PR. Let's make this smaller change first!

---

When cabal test and the error message recommends setting tests: True, the next cabal test run is likely to fail with a solver error. The user might incorrectly conclude that setting tests: True made the problem worse, because the failure now occurs earlier (at solving time rather than at testing time).

By including the fact that a plan failure is expected in the error message, hopefully users will be more confident that setting tests: True was the right move, so they will be able to focus on the true cause of the problem: the fact that no plan including the tests exist.

---

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!

[Mikolaj]

LGTM.

[Mikolaj]

BTW, in the ideal world, we'd also detect that tests: False is set (I think that's quite common for whatever reasons) and have a different message for that. In many cases everything works fine after removing tests: False [edit: and so having the third, distinct, behaviour], but people are instead advised to do tests: True that slows down cabal build when tests are not needed and is then reverted by the people to tests: False to cause panic later on.

[gelisam]

Please also shortly describe how you tested your change.

I cd'd to the test I created as part of the larger PR, and I removed package-with-buildable-test from cabal.project so that cabal test would fail with the "none of the components are available to build" error message. It indeed printed my updated error message:

OnlyBuildableTests $ ../../../../../dist-newstyle/build/x86_64-linux/ghc-8.10.7/cabal-install-3.7.0.0/x/cabal/build/cabal/cabal test all
Warning: /home/gelisam/.cabal/config: Unrecognized field world-file on line 29
Warning: The package list for 'hackage.haskell.org' is 24 days old.
Run 'cabal update' to get the latest list of available packages.
Resolving dependencies...
Error: cabal: Cannot test all the packages in the project because none of the
components are available to build: the test suite 'unbuildable-test' is not
available because the solver picked a plan that does not include the test
suites, perhaps because no such plan exists. To see the error message
explaining the problems with such plans, force the solver to include the test
suites for all packages, by adding the line 'tests: True' to the
'cabal.project.local' file.

[gelisam]

BTW, in the ideal world, we'd also detect that tests: False is set

Good news, that's already the case! If I set tests: False in the test setup I've described above, I get a different error message:

OnlyBuildableTests $ echo "tests: False" > cabal.project.local 
monolith OnlyBuildableTests $ ../../../../../dist-newstyle/build/x86_64-linux/ghc-8.10.7/cabal-install-3.7.0.0/x/cabal/build/cabal/cabal test all
Error: cabal: Cannot test all the packages in the project because none of the
components are available to build: the test suite 'unbuildable-test' is not
available because building test suites has been disabled in the configuration

[Mikolaj]

Whoa! We live in the ideal world!

Edit: Though perhaps it would be clearer with "has been disabled in the configuration (e.g., by a project configuration file setting 'tests: False' and, if so, you may try removing it).". OTOH, longer error messages are more likely not be read at all.

[Kleidukos]

@Mikolaj This begs for a "Quick-fix: Remove Tests: False from cabal.project.local" line at the bottom (and maybe generalise those in a separate line so that the eye of the reader can scan the output more rapidly?)

[Mikolaj]

@gelisam: your call how exactly it's going to look in the end. Please rebase and merge when ready.

[gelisam]

I don't think Quick-fix: Remove Tests: False from cabal.project.local is the right message, because it could also be in ~/.cabal/config or a --enable-test from the command-line. It would certainly be possible to detect which ones of those are involved and tell the user which one to quick fix, but that would be more work, so let's do that in a separate PR. Merging!

[Kleidukos]

@gelisam Yep, file detection is a must-have if we want to pull off such a trick :)

[gelisam]

Huh, turns out earlier in the file, there are already some error messages with very good explanations, for both the tests: False and the no-test-flag-and-no-plan-found cases:

cabal/cabal-install/src/Distribution/Client/CmdErrorMessages.hs

Lines 252 to 276 in cc59881

	renderTargetProblem verb _ (TargetOptionalStanzaDisabledByUser _ cname _) =
	"Cannot " ++ verb ++ " the " ++ showComponentName cname ++ " because "
	++ "building " ++ compkinds ++ " has been explicitly disabled in the "
	++ "configuration. You can adjust this configuration in the "
	++ "cabal.project{.local} file either for all packages in the project or on "
	++ "a per-package basis. Note that if you do not explicitly disable "
	++ compkinds ++ " then the solver will merely try to make a plan with "
	++ "them available, so you may wish to explicitly enable them which will "
	++ "require the solver to find a plan with them available or to fail with an "
	++ "explanation."
	where
	compkinds = renderComponentKind Plural (componentKind cname)
	renderTargetProblem verb _ (TargetOptionalStanzaDisabledBySolver pkgid cname _) =
	"Cannot " ++ verb ++ " the " ++ showComponentName cname ++ " because the "
	++ "solver did not find a plan that included the " ++ compkinds
	++ " for " ++ prettyShow pkgid ++ ". It is probably worth trying again with "
	++ compkinds ++ " explicitly enabled in the configuration in the "
	++ "cabal.project{.local} file. This will ask the solver to find a plan with "
	++ "the " ++ compkinds ++ " available. It will either fail with an "
	++ "explanation or find a different plan that uses different versions of some "
	++ "other packages. Use the '--dry-run' flag to see package versions and "
	++ "check that you are happy with the choices."
	where
	compkinds = renderComponentKind Plural (componentKind cname)

Maybe a better fix would have been to copy this text from the TargetOptionalStanzaDisabledBy{User,Solver} cases to the TargetDisabledBy{User,Solver} cases? Looks like the former is displayed when we request a specific test, while the later is displayed when we use a target which could theoretically include more than one test:

$ cabal test package-with-buildable-test:test:buildable-test --disable-tests
Warning: The package list for 'hackage.haskell.org' is 35 days old.
Run 'cabal update' to get the latest list of available packages.
Resolving dependencies...
cabal: Cannot test the test suite 'buildable-test' because building test
suites has been explicitly disabled in the configuration. You can adjust this
configuration in the cabal.project{.local} file either for all packages in the
project or on a per-package basis. Note that if you do not explicitly disable
test suites then the solver will merely try to make a plan with them
available, so you may wish to explicitly enable them which will require the
solver to find a plan with them available or to fail with an explanation.

$ cabal test package-with-buildable-test:tests --disable-tests
cabal: Cannot test the test suites in the package
package-with-buildable-test-1.0 because none of the components are available
to build: the test suite 'buildable-test' is not available because building
test suites has been disabled in the configuration

[Mikolaj]

A good idea IMHO. That would be in addition to what's done in this PR, right? Nothing would be reverted?

[gelisam]

The message for TargetOptionalStanzaDisabledBy{User,Solver} already explain the situation very well, I don't think the explanation I added in this PR adds anything on top of that. So I was thinking of reverting this PR and replacing it with a copy of the TargetOptionalStanzaDisabledBy{User,Solver} text. Or even better, to move these texts to helper functions, so they can be reused in both the single target and multiple target error messages.

[Mikolaj]

I think the explanation you've found covers mostly the case of explicit tests: False ("explicitly disabled in the configuration") and yours mostly the case of "can't construct a plan, probably due to deps". I'd rather merge both aspects in one message and, as you say, have a helper function and use it in all relevant places.

[gelisam]

I think the explanation you've found covers mostly the case of explicit tests: False ("explicitly disabled in the configuration")

I disagree! Note that I have found two messages, and that there are four relevant messages in total.

1. TargetDisabledBySolver is the message I've updated in this PR. it's indeed about the no-test-flag-and-no-plan-found case.
2. TargetDisabledByUser, just above TargetDisabledBySolver, is an existing message which covers the tests: False case, but the message only explains that tests are disabled, it doesn't explain how to enable them. I didn't update it in this PR because figuring out which file or command-linev argument led to the tests being disabled is not trivial.
3. TargetOptionalStanzaDisabledByUser is another existing message which also covers the tests: False case. It's a much better message: it specifies which configuration file disabled the test, it warns that commenting out the line might not be enough because it will let the solver decide, better to explicitly enable the tests, and warns that doing so could lead to a new error message. One potential problem is that it always says that the tests have been disabled in cabal.config{,.local}, but that's not always correct, they could also be disabled in ~/.cabal/config or the command-line.
4. TargetOptionalStanzaDisabledBySolver is an existing message which covers the no-test-flag-and-no-plan-found case. Did you miss it? It's in the second half of the code snippet, you have to scroll down to see it. It explains that the solver didn't find a place, it suggests enabling tests in the cabal.config.local file, and warns that this might fail with an error message if the solver still can't find a build plan. The only thing missing is that it doesn't specify that the way to enable tests in the cabal.config.local file is by adding the line tests: True to it.

[Mikolaj]

You are absolutely right. The messages you've found are better than whatever we've brainstormed here (though perhaps can be slightly improved based on our attempts). Indeed I didn't scroll and just read the example message you provided, thinking the source code you've found generates only that. I think we can either revert or remove the changelog from this PR in the amendment PR. I don't have a preference.