Skip to content

Commit

Permalink
Support detailed-0.9 tests commercialhaskell#1429
Browse files Browse the repository at this point in the history
  • Loading branch information
luigy committed Nov 28, 2015
1 parent d6dae2a commit 04f6d12
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 6 deletions.
2 changes: 2 additions & 0 deletions ChangeLog.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ Other enhancements:
* Support git-style executable fall-through (`stack something` executes
`stack-something` if present)
[#1433](https://github.com/commercialhaskell/stack/issues/1433)
* Support `detailed-0.9` tests
[#1429](https://github.com/commercialhaskell/stack/issues/1429)

Bug fixes:

Expand Down
28 changes: 23 additions & 5 deletions src/Stack/Build/Execute.hs
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ import Data.Text.Encoding (decodeUtf8)
import Data.Time.Clock (getCurrentTime)
import Data.Traversable (forM)
import Data.Word8 (_colon)
import qualified Distribution.PackageDescription as C
import Distribution.System (OS (Windows),
Platform (Platform))
import qualified Distribution.Text
Expand Down Expand Up @@ -1148,7 +1149,7 @@ singleTest runInBase topts testsToRun ac ee task installedMap = do
-- FIXME: Since this doesn't use cabal, we should be able to avoid using a
-- fullblown 'withSingleContext'.
(allDepsMap, _cache) <- getConfigCache ee task installedMap True False
withSingleContext runInBase ac ee task (Just allDepsMap) (Just "test") $ \package _cabalfp pkgDir _cabal announce _console mlogFile -> do
withSingleContext runInBase ac ee task (Just allDepsMap) (Just "test") $ \package cabalfp pkgDir _cabal announce _console mlogFile -> do
config <- asks getConfig
let needHpc = toCoverage topts

Expand Down Expand Up @@ -1178,11 +1179,24 @@ singleTest runInBase topts testsToRun ac ee task installedMap = do
_ -> ""

errs <- liftM Map.unions $ forM testsToRun $ \testName -> do
nameDir <- parseRelDir $ T.unpack testName
nameExe <- parseRelFile $ T.unpack testName ++ exeExtension
nameTix <- liftM (pkgDir </>) $ parseRelFile $ T.unpack testName ++ ".tix"
suites <- fmap (C.condTestSuites . snd) (readPackageUnresolved cabalfp)

This comment has been minimized.

Copy link
@mgsloan

mgsloan Nov 30, 2015

I think it would make sense to instead add more information to Package, and modify resolvePackage to provide it. That way, cabal conditionals will be resolved properly.

Maybe change the type of packageTests to Map Text TestSuiteInterfact, and use keysSet in spots where other usages of it need a Set?

This comment has been minimized.

Copy link
@luigy

luigy Nov 30, 2015

Author Owner

Yes, I'm in favor of this 👍

This comment has been minimized.

Copy link
@luigy

luigy Nov 30, 2015

Author Owner

Cool so there was no need to parse the cabal file again. package already has that information after updating packageTests to a Map Text TestSuiteInterface


let stestName = T.unpack testName
mtestSuiteInterface = fmap (C.testInterface . C.condTreeData . snd)
$ find ((== stestName) . fst) suites
(testName', isTestTypeLib) =
case mtestSuiteInterface of
Just C.TestSuiteLibV09{} -> (stestName ++ "Stub", True)
Just C.TestSuiteExeV10{} -> (stestName, False)
_ -> ("", False) -- TODO throw an unsupported error or not found?

nameDir <- parseRelDir $ testName'
nameExe <- parseRelFile $ testName' ++ exeExtension
nameTix <- liftM (pkgDir </>) $ parseRelFile $ stestName ++ ".tix"
let exeName = buildDir </> $(mkRelDir "build") </> nameDir </> nameExe

exists <- fileExists exeName

menv <- liftIO $ configEnvOverride config EnvSettings
{ esIncludeLocals = taskLocation task == Local
, esIncludeGhcPackagePath = True
Expand All @@ -1205,7 +1219,7 @@ singleTest runInBase topts testsToRun ac ee task installedMap = do
announce $ "test (suite: " <> testName <> argsDisplay <> ")"
let cp = (proc (toFilePath exeName) args)
{ cwd = Just $ toFilePath pkgDir
, Process.env = envHelper menv
, Process.env = Just $ ("HPCTIXFILE", toFilePath nameTix) : maybe [] id (envHelper menv)

This comment has been minimized.

Copy link
@mgsloan

mgsloan Nov 30, 2015

This is an enhancement independent of support for detailed-0.9, right? Seems sensible to me, though.

This comment has been minimized.

Copy link
@luigy

luigy Nov 30, 2015

Author Owner

One issue I ran into without this is that it would require updating generateHpcReport to determine the correct tix filepath based on testSuiteInterface again as done above for exeName. Extending packageTests to have that extra information like you mentioned would help with this

This comment has been minimized.

Copy link
@mgsloan

mgsloan Nov 30, 2015

How does nameTix depend on testSuiteInterface? Seems to me like it's just a function of stestName / testName

Or maybe it's that nameTix should use testName'?

This comment has been minimized.

Copy link
@luigy

luigy Nov 30, 2015

Author Owner

generateHpcReport only has access to test names (in ex. testname) and for detailed-09 test the tix file has to take into account the generated testStub filepath. I remember changing only nameTix to use either name resulted in a broken updateTix or generateHpcReport

The env var was a quick way to have guarantee of the filepath to get it work, but extending packageTests to have that extra information would help out updating Coverage to better support different test-suites

This comment has been minimized.

Copy link
@luigy

luigy Nov 30, 2015

Author Owner

Now I realize that I was confusing testPackages as being of some type of NamedComponents... Hmm, so it still requires to use resolvePackage to have that information available so I guess using the env var looks like a cleaner solution :P

This comment has been minimized.

Copy link
@luigy

luigy Nov 30, 2015

Author Owner

Hmm does testsToRun == Map.keys (packageTests package) ?

That way packageTests can be passed to Coverage functions instead to handle tix file manipulation correctly

This comment has been minimized.

Copy link
@mgsloan

mgsloan Dec 1, 2015

Nope, it's testComponents (taskComponents task), so probably best to leave things as-is.

, std_in = CreatePipe
, std_out =
case mlogFile of
Expand All @@ -1219,6 +1233,10 @@ singleTest runInBase topts testsToRun ac ee task installedMap = do

-- Use createProcess_ to avoid the log file being closed afterwards
(Just inH, Nothing, Nothing, ph) <- liftIO $ createProcess_ "singleBuild.runTests" cp
when isTestTypeLib $ liftIO $ do
let testLog = buildDir </> $(mkRelFile "stack-test.log")
removeFileIfExists testLog
hPutStr inH $ show (testLog, testName)
liftIO $ hClose inH
ec <- liftIO $ waitForProcess ph
-- Move the .tix file out of the package
Expand Down
2 changes: 1 addition & 1 deletion src/Stack/Types/Build.hs
Original file line number Diff line number Diff line change
Expand Up @@ -725,7 +725,7 @@ configureOptsNoDir econfig bco deps wanted isLocal package = concat
allGhcOptions = concat
[ Map.findWithDefault [] Nothing (configGhcOptions config)
, Map.findWithDefault [] (Just $ packageName package) (configGhcOptions config)
, concat [["-fhpc", "-fforce-recomp"] | isLocal && toCoverage (boptsTestOpts bopts)]
, concat [["-fhpc"] | isLocal && toCoverage (boptsTestOpts bopts)]

This comment has been minimized.

Copy link
@mgsloan

mgsloan Nov 30, 2015

Ah, yeah, I don't think I intended to commit the addition of "-fforce-recomp", good catch! Related to commercialhaskell#1411

, if includeExtraOptions
then boptsGhcOptions bopts
else []
Expand Down

0 comments on commit 04f6d12

Please sign in to comment.