ekmett / distributive

Dual Traversable
http://hackage.haskell.org/package/distributive
Other
41 stars 25 forks source link

Remove dependency on cabal-doctest #38

Closed tfausak closed 7 years ago

tfausak commented 7 years ago

This fixes #37. It also switches to the lts-9.0 resolver because it wouldn't build with nightly-2016-01-08.

RyanGlScott commented 7 years ago

This will break the doctests, and more importantly, this isn't the solution to the problem you're facing. See my comments here.

tfausak commented 7 years ago

This will break the doctests

Then the build might not be configured properly.

screen shot 2017-09-29 at 9 37 13 am

Regardless, the tests do fail for me locally. Fixing them is pretty easy:

diff --git a/tests/doctests.hs b/tests/doctests.hs
index 2d080e7..1a136d7 100644
--- a/tests/doctests.hs
+++ b/tests/doctests.hs
@@ -13,13 +13,8 @@
 -----------------------------------------------------------------------------
 module Main where

-import Build_doctests (flags, pkgs, module_sources)
-import Data.Foldable (traverse_)
 import Test.DocTest

 main :: IO ()
 main = do
-    traverse_ putStrLn args
-    doctest args
-  where
-    args = flags ++ pkgs ++ module_sources
+    doctest ["src"]

I don't grok cabal-doctest. What does it do?

RyanGlScott commented 7 years ago

Indeed, I'm quite surprised that Travis is green there, since cabal test definitely failed here.

In any case, I encourage reading cabal-doctest's README for an overview of what it does. It solves many common pratfalls that can befall you when using doctest at scale. For example, if you have two packages installed that both define a module named Foo and attempt to import Foo somewhere in your project, then running doctest naked on your code will fail, since it won't be able to disambiguate between the two packages that provide Foo. The solution is to pass an explicit package ID an an argument to doctest to resolve the ambiguity, a tiresome task which cabal-doctest automates by gathering the appropriate info during the configure phase of Cabal.

Yes, it's quite annoying that all of this requires a custom Setup script to accomplish. There are plans to make Cabal aware of doctest in the future, so one day we will be able to axe this Setup shim entirely. But until then, we must do what we can.

As I said in https://github.com/ekmett/distributive/issues/37, the best solution to the particular problem you're facing (using Cabal HEAD) is to bump the upper version bounds on Cabal in cabal-doctest to allow it.

tfausak commented 7 years ago

I think the build succeeds because the test script just runs a bunch of commands without checking the exit codes. They would need to be joined by && for the comment above them to be accurate.

I did read cabal-doctest's README, but it wasn't clear to me what it actually does. Thanks for explaining it. For this particular case, there's only a single doctest and it works fine without cabal-doctest. Is cabal-doctest really necessary here?

RyanGlScott commented 7 years ago

cabal-doctest isn't strictly necessary for testing distributive in isolation on Travis since the only packages we install are precisely its dependencies, so there's no worry of global module conflicts as described above. But there's no guarantee that this will hold if someone attempts to test distributive on their machine, which might have a substantially larger package database. Indeed, this is a problem that Stackage (which maintains an extraordinarily larges package database) has to deal with regularly. See here and here for examples of where this has bitten.

So yes, cabal-doctest is necessary to ensure that the tests will work reliably with different varieties of configurations.