Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inter-test dependencies #48

Closed
UnkindPartition opened this issue Jan 29, 2014 · 58 comments
Closed

Inter-test dependencies #48

UnkindPartition opened this issue Jan 29, 2014 · 58 comments

Comments

@UnkindPartition
Copy link
Owner

@jstolarek writes:

I want to run test A first and if it succeeds I want to run test B (if A fails B is skipped).

@joeyh has also requested this feature in #47.

The main problem here is a syntax/API for declaring dependencies. It would be relatively easy for hspec-style monadic test suites, but I don't see a good way to do it using the current list-based interface.

(This could be a reason to move to the monadic interface. But it's a big change, and I'd like to explore other options first.)

@jstolarek
Copy link

Java does this using annotations (see here).

@UnkindPartition
Copy link
Owner Author

I see. Well, we could also refer to other tests by their names, I guess.

The syntax then would be something like

tests = testGroup "Tests"
  [ testCase "bar" ...
  , ifSucceeds "bar" $ testCase "foo" ...
  ]

ifSucceeds should interpret its argument as a pattern, I think.

The monadic syntax would be, respectively,

tests = testGroup "Tests" $ do
  bar <- testCase "bar" ...
  ifSucceeds bar $ testCase "foo" ...
  • The string-based syntax is, of course, prone to typos and fragile due to test renamings
  • The monadic syntax requires the test and its dependencies to be in the same block. Even though it's possible to return test identifiers from do-blocks, it's not very convenient, especially when there's several of such identifiers
  • There's also some repetition in the monadic syntax: we have to give a name to both the test ("bar") and its identifier (bar)

Other thoughts?

Also, what should the semantics be in the case when the dependency is excluded (e.g. by a pattern), but the dependent test is included? I think the depending test should still run. The logic is that presumably if the dependency fails, the dependent test would certainly fail, too, but since we don't know whether the dependency failed or not, there's no reason not to try the dependent test.

@sjoerdvisscher
Copy link

With the string-based syntax you could check all references before running any tests. Then typos would not be a problem in practice.

@UnkindPartition
Copy link
Owner Author

@sjoerdvisscher good point!

@jstolarek
Copy link

I agree with the reasoning about dependent tests.

Oh wait, can I exclude some tests from running in tasty? I wasn't aware of that.

@UnkindPartition
Copy link
Owner Author

@jstolarek yes, see http://documentup.com/feuerbach/tasty#options/patterns (you may want to skim through the rest of the document, too :)

@joeyh
Copy link

joeyh commented Jan 29, 2014

If A creates a resource, before B uses it, and C cleans it up,
can a dependency ensure C is run any time A is run?
This might be a desirable property. OTOH, this is duplicating part of
resources (but as I mentioned in #47, it made sense for me to want to
run some tests as part of setting up a complicated resource).

Also, this may impact tasty-rerun. If B failed before, but A succeeded,
I'd want a rerun to still run A again, before rerunning B.

see shy jo

@esmolanka
Copy link

The question is: should dependencies make arbitrary DAG or just a tree? (I guess, the former)

  • DAGs could be made with monadic approach and it's almost free, but there is pointed out earlier redundant names problem (foo <- testCase "foo");
  • Trees can be made with some clever combinators, it's also free and test references are anonymous, but this language could be too weak for some more complex cases;
  • String-typed references IMO are bad: need of tracking graph cycles, doing topological sorting, name checking and this is for a single benefit: no name duplication.

Example combinator for trees:

tests = testGroup "Tests"
  [ testCase "bar" ...
    |>  -- `ifSucceedsThen` 
       [ testCase "foo" ...
       , ...
       ]
  ]

I would try to do combination of monadic and combinator approach.

@UnkindPartition
Copy link
Owner Author

@joeyh having an edge type for ordering (run C no earlier than A or B finish) sounds reasonable.

Having an edge type that reverses the effect of a pattern (that may happen to exclude C but not A) — a bit less so.

(I'm trying to think whether these features make sense generally, outside of this particular use case.)

If I could replace tasty's resources with dependencies, it would be great, but it doesn't seem practically possible for many reasons.

@UnkindPartition
Copy link
Owner Author

@esmolanka I think you're talking not about DAGs vs trees (which doesn't matter here much — DAGs are fine), but about dependencies following the structure of the test tree itself, which I find too restrictive. Or maybe I just don't understand your point at all.

Topological sorting is necessary in any case.

and this is for a single benefit: no name duplication

Not at all. The main reason is that I don't want to change the main list-based syntax, because it would a rather significant change. Also, as I said above, passing identifiers out of a do-block is inconvenient.

@jstolarek
Copy link

I think the problem is complicated by the fact that tests are already organized in a tree, while adding test dependencies organizes them into a dependency graph. So on the one hand there is this graph and on the other the test hierarchy.

My question is: what is the motivation behind organizing tests in a tree as they are now? I presume that this is supposed to reflect logical structure of tests as perceived by the user but the truth is that if there are no dependencies between tests they don't need to be in a tree.

@UnkindPartition
Copy link
Owner Author

Yes, that's just for the user convenience — for the same reasons we organize files into directories, although technically it's not necessary.

Also, it makes it easier to exclude or include a subtree for running, apply options to a subtree etc.

@ocharles
Copy link
Collaborator

ocharles commented Feb 2, 2014

I don't like the idea of doing this based on strings, so I would be in-favour of @feuerbach's original monad proposal. This feels to be the most type-safe and scope-safe way of doing this. The worry about a name being required might not actually be so bad in practice - if you can get a whole testGroup to depend on just one testCase, then maybe you can use composition to transparently pass the name through (like how we use >>=).

@UnkindPartition
Copy link
Owner Author

@ocharles could you explain what in particular you don't like about strings?

Here's a less stringy (but still dynamic) alternative: mark tests with values of any type you want (as soon as it's Typeable and Eq, and maybe Ord for efficiency), and refer to them by that identifier.

The worry about a name being required might not actually be so bad in practice

As I said above, this is not my biggest worry (just a nice bonus of not going monadic).

The main reason is that I don't want to change the main list-based syntax, because it would a rather significant change. Also, passing identifiers out of a do-block is inconvenient.

if you can get a whole testGroup to depend on just one testCase, then maybe you can use composition to transparently pass the name through

We can have such a combinator, too. So as soon as it is sufficient, you can have a perfectly static graph without any identifiers.

@UnkindPartition
Copy link
Owner Author

Here's another angle to look at this problem at.

It seems to me that the majority of use cases follow the pattern: perform a linear sequence of actions (with possible data dependencies between them); while we're doing so, perform a number of checks and display the result of each check as a separate test.

If that is all we need, then perhaps we can do it easier than arbitrary test dependencies?
Something much like HUnit, where you can have multiple assertions per Assertion. Only in our case the successful assertions (and the first unsuccessful one) will render as separate tests in tasty.

It would look like:

testCase "Compound test" $ do
  a <- liftIO actionA
  check "a is good" (checkA a)
  b <- liftIO (actionB a)
  check "b is good" (checkB b)

And in the output you could see

Compound test
  a is good [OK]
  b is good [OK]

These subtests wouldn't be first-class: they are simple assertions (so they can't be quickcheck tests or test groups); they can't be selected with patterns. I think this is reasonable.

Are there any use cases that this approach wouldn't cover? (Or any other objections?)

(Hm, maybe this is what everyone was talking about, but I was too blind to see it?)

@UnkindPartition
Copy link
Owner Author

In other words, this will be just another test provider (essentially an HUnit replacement). All that will have to be done in tasty itself is just support for displaying dynamic sub-tests/assertions.

@ocharles
Copy link
Collaborator

ocharles commented Feb 5, 2014

I'm not quite sure how that's different to HSpecs, it type stuff. Could you elaborate on how it's different?

@UnkindPartition
Copy link
Owner Author

Does it have to be different? :) Indeed, it looks quite similar.

@jstolarek
Copy link

they are simple assertions (so they can't be quickcheck tests or test groups)

I don't like that restriction. If I want to run a number of tests and only if all of them succeed run a particular test then I cannot organizy my tests into groups - I am forced to flatten the tree test into tree list. OTOH I'd rather have such feature now than something more sophisticated in a couple of months.

Are there any use cases that this approach wouldn't cover?

TBH I'm not sure about my use case. I want to test whether a program runs (with tasty-program) and if the program runs successfully I want to check whether output that it produced is exactly as expected (with tasty-golden).

@UnkindPartition
Copy link
Owner Author

If I want to run a number of tests and only if all of them succeed run a particular test then I cannot organizy my tests into groups - I am forced to flatten the tree test into tree list.

Can you give me a convincing practical example of such a case?

OTOH I'd rather have such feature now than something more sophisticated in a couple of months.

The simple version that I propose could be ready in a couple of months (unless someone else wants to do the work). Something graph-like would take much longer.

I want to test whether a program runs (with tasty-program) and if the program runs successfully I want to check whether output that it produced is exactly as expected (with tasty-golden).

I don't think this is a good idea. Why do you need these to be separate tests? Why not simply do this through the goldenTest function?

Golden tests are mostly about automated management of golden files. Suppose we want to update a golden file. How are we supposed to know that, in order to obtain the current output, we need to run those other tests?

@jstolarek
Copy link

Why do you need these to be separate tests?

I prefer to have a report from the testsuite that says running a program has failed and there was no attempt to test its output, rather than have a golden test fail and having to examine manually whether it happened because program could not be run successfully or it could be run successfully but it produced unexpected output. So having two separate tests would be more convenient for me. It's the same reason why you don't put multiple assertions into a single unit test - atomicity of the test.

@UnkindPartition
Copy link
Owner Author

If the program fails, just make the error message say so. Would it be not clear enough?

Specifically, instantiate a in the type of goldenTest with Maybe ByteString. Make the value getter return Nothing when the program fails. Produce the appropriate error message in the comparison function.

@ozgurakgun
Copy link
Contributor

Any updates on this? If needed, I am happy to work on this because my tests for a project are taking quite some time, they are almost embarrassingly parallel, and it is becoming embarrassing to run them on a 32-core machine with no parallelism. :)

Cheers!

@UnkindPartition
Copy link
Owner Author

@ozgurakgun can you describe your use case in more detail? what kind of tests are these, what dependency (and why) is there?

@ozgurakgun
Copy link
Contributor

@feuerbach thanks for the quick reply.

I guess the easiest way to describe my setup will be staying as closely as possible to the code.

testSpecs :: [(A, [B], [C])]
testA :: A -> TestTree
testB :: B -> TestTree
testC :: C -> TestTree

allTests :: TestTree
allTests = testGroup "all"
    [ testGroup "some_name" (testA a : map testB bs : map testC cs)
    | (a, bs, cs) <- testSpecs
    ]

Here, each entry in the top-level list is independent. They can be run in parallel.

In each entry, A needs to run first, then Bs can be run in parallel, then Cs can be run in parallel.

I guess what I need is a variation of the testGroup combinator, which will run the test trees in the list sequentially, exploiting parallelism for each item before starting the next. This idea is very similar to what has been suggested before in this thread. It may require restructuring the test specification in certain cases, but I think that is acceptable. At least in my current case.

I would be happy if I could write the following.

allTests :: TestTree
allTests = testGroup "all"
    [ testGroup_Sequentially "some_name" [testA a, bGroup, cGroup]
    | (a, bs, cs) <- testSpecs
    , let bGroup = testGroup "bs" (map testB bs)
    , let cGroup = testGroup "cs" (map testC cs)
    ]

(Please someone find a better name for testGroup_Sequentially!)

@UnkindPartition
Copy link
Owner Author

But why is there a dependency? Is it because Bs and Cs depend on the side effects of As? Or does A test some pre-condition, and if it fails, there's no point in running Bs and Cs?

@ozgurakgun
Copy link
Contributor

Actually, both. A produces some files as output, which are required for Bs. Hence, if A fails Bs and Cs do not need to be run. (Though I don't care too much if they are run anyway.)

@UnkindPartition
Copy link
Owner Author

I see.

Here's how I see the design. First of all, I'm very reluctant at this point to make breaking changes in the core interface. Thus we should refer to other tests by their names, and there should be combinators along the lines of runBefore "test1", runAfter "test2" etc. These combinators create metainformation that is used to sort the tests topologically and then execute them in that order, also exploiting parallelism.

Also, as suggested by Sjoerd, there should be a check somewhere in the beginning that all references actually resolve.

I don't have resources to work on this atm (and in the foreseeable future), but you can totally try it yourself. I'm happy to review your early designs/prototypes.

@mightybyte
Copy link

I think I agree with @ocharles that the monadic interface feels like the way to go here. It seems like we need the most powerful abstraction possible at the lowest level, otherwise we keep end up running into these situations where as Ed says, "we can't say all the things".

@RyanGlScott
Copy link
Contributor

This looks extremely cool, @feuerbach. I'm eager to incorporate this feature into the singletons test suite (which would greatly benefit from this feature).

I've been playing around with the dependencies branch for a bit and trying out some toy examples. One thing that's unclear to me (and perhaps you intend to document this) is what happens if you declare recursive dependencies, such as in the following example:

{-# LANGUAGE NumDecimals #-}
import Test.Tasty
import Test.Tasty.HUnit
import Control.Concurrent

main :: IO ()
main = defaultMain $ testGroup "Tests" $
  [ after AllSucceed "Foo" $ testCase "Foo" $ threadDelay 1e6
  ]

Currently, this loops infinitely at runtime (instead of, say, throwing an error). Is this the intended behavior?

@UnkindPartition
Copy link
Owner Author

Good catch Ryan, should be fixed now.

@RyanGlScott
Copy link
Contributor

Thanks @feuerbach!

I've encountered another awkward design consideration that I'm not sure how to address. Here is some code from the singletons test suite (simplified for presentation purposes):

testGroup "Database client"
    [ testCase "Database" $ threadDelay 1e6
    , testCase "Main" $ threadDelay 1e6
    ]

I want the Main test case to run only after the Database test case has finished. I attempted this using after like so:

testGroup "Database client" $
    [ testCase "Database" $ threadDelay 1e6
    , after AllFinish "Database" $
      testCase "Main" $ threadDelay 1e6
    ]

Unfortunately, this doesn't quite do what I would have hoped:

λ> main
*** Exception: Test dependencies form a loop.

After some head-scratching, I realized that the Database pattern was matching the entire Database client test group, which resulted in cyclic dependencies. On the other hand, this wasn't my intention—I wanted to match the Database test case in particular. Is there a way that I can communicate this fact to tasty?

@UnkindPartition
Copy link
Owner Author

UnkindPartition commented Sep 27, 2018

Yes, you can do that since tasty patterns are awk expressions. Instead of "Database", try something like "$3 == \"Database\"", where 3 is the level at which that test group/case occurs in the tree (the outermost group is number 1). See https://github.com/feuerbach/tasty/blob/master/README.md#patterns for the details.

@RyanGlScott
Copy link
Contributor

Ah, I completely missed that, thanks.

A design question: what is the best idiom for depending on multiple tests? Here are two semantically equivalent TestGroups:

main :: IO ()
main = defaultMain $
  testGroup "Tests" $
  [ testGroup "Group1"
    [ after AllSucceed "Bar" $
      after AllSucceed "Baz" $
      testCase "Foo" $ threadDelay 1e6
    ]
  , testGroup "Group2"
    [ testCase "Bar" $ threadDelay 1e6
    , testCase "Baz" $ threadDelay 1e6
    ]
  ]
main :: IO ()
main = defaultMain $
  testGroup "Tests" $
  [ testGroup "Group1"
    [ after AllSucceed "$3 == \"Bar\" || $3 == \"Baz\"" $
      testCase "Foo" $ threadDelay 1e6
    ]
  , testGroup "Group2"
    [ testCase "Bar" $ threadDelay 1e6
    , testCase "Baz" $ threadDelay 1e6
    ]
  ]

I'm not sure if one of these idioms parallelizes better than the other, though. (I could imagine that stacking uses of after might affect how things are scheduled.) Do you suspect this to be the case? If so, do you think after should take a list of patterns instead of just a single one?

@UnkindPartition
Copy link
Owner Author

UnkindPartition commented Sep 28, 2018

There's no semantic difference between the two styles—in both cases parallelism is exploited to the maximum extent allowed by the dependencies—so feel free to use the style you find more readable.

There is however a semantic difference between "Bar" and "$3 == \"Bar\"". A more accurate translation of your first example would be /Bar/ || /Baz/.

@RyanGlScott
Copy link
Contributor

Good to know, thanks. I was able to port over the entirety of the singletons test suite to express all of its test dependencies. Now I can finally run those tests in parallel! (On my machine using four threads, this reduces the total run time from about 90 seconds to 30 seconds, which is pretty darn cool.)

I suppose if I had one final comment to make about the proposed API, it's that I had to be extremely cautious when typing in the patterns it depended on, since it's possible to "depend" on tests which don't exist. For example:

main :: IO ()
main = defaultMain $
  testGroup "Tests" $
  [ after AllSucceed "Baz" $
    testCase "Foo" $ threadDelay 1e6
  , testCase "Bar" $ threadDelay 1e6
  ]

The Foo test case runs without issue, despite the fact that no tests match the Baz pattern. I suppose one could argue that this makes sense since Foo did in fact wait for all of its dependencies to succeed (all zero of them) before it ran. On the other hand, it does mean that if one were to change the name of a test which had reverse dependencies and forgot to update the uses of after which depended on that test's old name, then one might not realize their mistake.

I wouldn't be too bothered either way on this point, but I thought it was worth pointing out.

@UnkindPartition
Copy link
Owner Author

Yeah, I thought about this as well. One solution I considered was a version of the after combinator which would take the expected number of the tests matching the pattern, and it would error if the wrong number of tests matched the pattern.

I decided not to implement it yet because it's not clear how many people would actually care to use this feature. But if enough people request it, I might add it.

@martijnbastiaan
Copy link
Contributor

Thanks for the hard work @feuerbach ! It's really nice to see this being addressed.

Unfortunately.. I'm having some issues incorporating this into the code I'm working on. Here's the context: I'm currently working on a Haskell to hardware compiler. To test the compiler we compile some designs to a number of target languages and run simulators on those languages to check if they have some expected output. A typical test case for one design looks something like:

I2C:
    VHDL:
        haskell-to-vhdl
        vhdl-compile
        vhdl-simulation
    Verilog:
        haskell-to-verilog
        verilog-compile
        verilog-simulation
    ......

where I2C is the program/circuit we're compiling, VHDL and Verilog are target languages, and the most inner steps are the actual test. We'd like to keep this granularity, so I previously hacked together a solution with locks and unsafe IO to make the inner tests run sequentially. In reality, we don't specify all these testcases manually. Instead, we only specify it on an I2C level and the actual tests are generated, as it would be tedious to manually add tests for vhdl/verilog/.. everytime. The code generating these tests ideally would also make the tests dependent on each other, such that they run sequentially.

Now.. I've actually written this code. For the following test cases:

Tests.Examples.I2C.VHDL.clash
Tests.Examples.I2C.VHDL.GHDL (library) [test_bitmaster]
Tests.Examples.I2C.VHDL.GHDL (library) [test_bytemaster]

It should run clash, then test_bitmaster, then test_bytemaster. So, my code generates the following patterns:

TestName Pattern
clash
GHDL (library) [test_bitmaster] ($(NF-0) == "clash") && ($(NF-1) == "VHDL") && ($(NF-2) == "I2C")
GHDL (library) [test_bytemaster] ($(NF-0) == "GHDL (library) [test_bitmaster]") && ($(NF-1) == "VHDL") && ($(NF-2) == "I2C")

But once I actually run the tests, the run in parallel! I tried changing the patterns to:

TestName Pattern
clash
GHDL (library) [test_bitmaster] ($(NF-0) == "clash") && ($(NF-1) == "VHDL")
GHDL (library) [test_bytemaster] ($(NF-0) == "GHDL (library) [test_bitmaster]") && ($(NF-1) == "VHDL")

At this point it the tests run sequentially as they should, but there's no need for GHDL (library) [test_bitmaster] to wait for all the other clash compilations! I've also tried the following patterns:

TestName Pattern
clash
GHDL (library) [test_bitmaster] ($(NF-0) == "clash") && ($(NF-2) == "I2C")
GHDL (library) [test_bytemaster] ($(NF-0) == "GHDL (library) [test_bitmaster]") && ($(NF-2) == "I2C")

which again runs in parallel. It seems to come down to $(NF-2), which just doesn't seem to work in dependent tests for some reason. Interestingly enough, if I use it as a pattern on the command line (with -p), they do match as I would expect!

Would you have any idea on where this might go wrong? Am I using the API incorrectly?

@UnkindPartition
Copy link
Owner Author

Thanks for trying this out, Martijn. I struggle a bit to understand what your test tree looks like. Could you try to create a minimal complete example that demonstrates your issue?

@martijnbastiaan
Copy link
Contributor

Yes, I should have done that in the first place. Setup:

module Main (main) where

import Control.Concurrent (threadDelay)

import Test.Tasty
import Test.Tasty.Clash
import Test.Tasty.HUnit

acquire :: IO ()
acquire = return ()

release :: a -> IO ()
release _ = return ()

The following executes as expected:

main :: IO ()
main = do
  defaultMain $ testGroup "L1"
    [ testGroup "L2"
      [ testCase "L2A" (threadDelay 1000000)
      , after AllFinish "($(NF-0) == \"L2A\") && ($(NF-1) == \"L2\") && ($(NF-2) == \"L1\")" $
          testCase "L2B" (threadDelay 1000000)
      ]
    ]

output:

L1
  L2
    L2A: OK (1.00s)
    L2B: OK (1.00s)

All 2 tests passed (2.00s)

Notice that it takes 2 seconds. Now the following:

main :: IO ()
main = do
  defaultMain $ testGroup "L1"
    [ withResource acquire release $ const $ testGroup "L2"
      [ testCase "L2A" (threadDelay 1000000)
      , after AllFinish "($(NF-0) == \"L2A\") && ($(NF-1) == \"L2\") && ($(NF-2) == \"L1\")" $
          testCase "L2B" (threadDelay 1000000)
      ]
    ]

output:

L1
  L2
    L2A: OK (1.00s)
    L2B: OK (1.00s)

All 2 tests passed (1.00s)

So maybe I'm simply using withResource incorrectly? It is supposed to setup some infrastructure (basically a number of mkdirs) and clean it out after the test finished (basically rm -rf).

@martijnbastiaan
Copy link
Contributor

Concerning the discussion about after: I think it should produce an error if it matched zero tests. It's hard to imagine a situation where you'd want to match zero tests, and I've been bitten by this multiple times now. Of course, some people might still be interested in matching zero or more tests, so an additional after0 could be added.

One solution I considered was a version of the after combinator which would take the expected number of the tests matching the pattern, and it would error if the wrong number of tests matched the pattern.

That would be great! In my case, the code generating the patterns knows perfectly well how many cases should be matched. Having this feature would decrease the possible number of name collisions in our testsuite to zero :). Though it must be said that this falls squarely into the category 'nice to have', not 'need to have' for my case.

@UnkindPartition
Copy link
Owner Author

I think it should produce an error if it matched zero tests

I think so too; unfortunately, it's a bit complicated to implement. The issue is that tests may be filtered, and when implemented naively, the error will trigger when the dependency is not included in the set of running tests. Implementing this properly requires some inconvenient bookkeeping. But I might get around to it some day.

@martijnbastiaan
Copy link
Contributor

That makes sense. Maybe I'll take a shot at it in a few weekends from now.

In case you missed it due to my second comment: I posted a minimal example of the bug (?) I'm experiencing. I've looked through the code added in the dependencies branch, but I haven't seen any obvious implementation mistakes yet.

@UnkindPartition
Copy link
Owner Author

Yes, I saw your example (thanks!) and can reproduce it — looks like a bug. I'll look into it.

@martijnbastiaan
Copy link
Contributor

martijnbastiaan commented Oct 18, 2018

I've done some digging. Test.Tasty.Pattern.exprMatches does not get the complete path sometimes. If the call comes from testPatternMatches it does. This leaves only one option as exprMatches is used in only one other place: resolveDeps. resolveDeps doesn't do anything funny with the path though, it just gets them from createTestActions. As we're only seeing the bug in combination with withResource, a likely source of the error is addInitAndRelease (use for foldResource) which might forget to pass the path-so-far along:

    addInitAndRelease :: ResourceSpec a -> (IO a -> Tr) -> Tr
    addInitAndRelease (ResourceSpec doInit doRelease) a = wrap $ do
      initVar <- atomically $ newTVar NotCreated
      (tests, fins) <- unwrap $ a (getResource initVar)
      let ntests = length tests
      finishVar <- atomically $ newTVar ntests
      let
        ini = Initializer doInit initVar
        fin = Finalizer doRelease initVar finishVar
        tests' = map (first $ local $ (Seq.|> ini) *** (fin Seq.<|)) tests
      return (tests', fins Seq.|> fin)

    wrap
      :: IO ([(InitFinPair -> IO (), (TVar Status, Path, Deps))], Seq.Seq Finalizer)
      -> Tr
    wrap = Traversal . WriterT . lift . fmap ((,) ())

    unwrap
      :: Tr
      -> IO ([(InitFinPair -> IO (), (TVar Status, Path, Deps))], Seq.Seq Finalizer)
    unwrap = flip runReaderT mempty . execWriterT . getTraversal

This is speculation though, as I'm having a hard time tracing/understanding due to all the abstractions going on.

@martijnbastiaan
Copy link
Contributor

This patch solves my problems:

diff --git a/core/Test/Tasty/Core.hs b/core/Test/Tasty/Core.hs
index 812e4f5..df75042 100644
--- a/core/Test/Tasty/Core.hs
+++ b/core/Test/Tasty/Core.hs
@@ -292,7 +292,7 @@ after deptype s =
 data TreeFold b = TreeFold
   { foldSingle :: forall t . IsTest t => OptionSet -> TestName -> t -> b
   , foldGroup :: TestName -> b -> b
-  , foldResource :: forall a . ResourceSpec a -> (IO a -> b) -> b
+  , foldResource :: forall a . ResourceSpec a -> Path -> (IO a -> b) -> b
   , foldAfter :: DependencyType -> Expr -> b -> b
   }
 
@@ -311,7 +311,7 @@ trivialFold :: Monoid b => TreeFold b
 trivialFold = TreeFold
   { foldSingle = \_ _ _ -> mempty
   , foldGroup = const id
-  , foldResource = \_ f -> f $ throwIO NotRunningTests
+  , foldResource = \_ _ f -> f $ throwIO NotRunningTests
   , foldAfter = \_ _ b -> b
   }
 
@@ -355,7 +355,7 @@ foldTestTree (TreeFold fTest fGroup fResource fAfter) opts0 tree0 =
         TestGroup name trees ->
           fGroup name $ foldMap (go pat (path Seq.|> name) opts) trees
         PlusTestOptions f tree -> go pat path (f opts) tree
-        WithResource res0 tree -> fResource res0 $ \res -> go pat path opts (tree res)
+        WithResource res0 tree -> fResource res0 path $ \res -> go pat path opts (tree res)
         AskOptions f -> go pat path opts (f opts)
         After deptype dep tree -> fAfter deptype dep $ go pat path opts tree
 
diff --git a/core/Test/Tasty/Run.hs b/core/Test/Tasty/Run.hs
index 27f302e..b564ce9 100644
--- a/core/Test/Tasty/Run.hs
+++ b/core/Test/Tasty/Run.hs
@@ -242,7 +242,7 @@ createTestActions opts0 tree = do
               Traversal $ local (second ((deptype, pat) :)) a
           }
         opts0 tree
-  (tests, fins) <- unwrap traversal
+  (tests, fins) <- unwrap mempty traversal
   let
     mb_tests :: Maybe [(Action, TVar Status)]
     mb_tests = resolveDeps $ map
@@ -263,10 +263,10 @@ createTestActions opts0 tree = do
         act (inits, fins) =
           executeTest (run opts test) statusVar (lookupOption opts) inits fins
       tell ([(act, (statusVar, path, deps))], mempty)
-    addInitAndRelease :: ResourceSpec a -> (IO a -> Tr) -> Tr
-    addInitAndRelease (ResourceSpec doInit doRelease) a = wrap $ do
+    addInitAndRelease :: ResourceSpec a -> Path -> (IO a -> Tr) -> Tr
+    addInitAndRelease (ResourceSpec doInit doRelease) p a = wrap $ do
       initVar <- atomically $ newTVar NotCreated
-      (tests, fins) <- unwrap $ a (getResource initVar)
+      (tests, fins) <- unwrap p $ a (getResource initVar)
       let ntests = length tests
       finishVar <- atomically $ newTVar ntests
       let
@@ -279,9 +279,10 @@ createTestActions opts0 tree = do
       -> Tr
     wrap = Traversal . WriterT . lift . fmap ((,) ())
     unwrap
-      :: Tr
+      :: Path
+      -> Tr
       -> IO ([(InitFinPair -> IO (), (TVar Status, Path, Deps))], Seq.Seq Finalizer)
-    unwrap = flip runReaderT mempty . execWriterT . getTraversal
+    unwrap p = flip runReaderT (p, mempty) . execWriterT . getTraversal
 
 -- | Take care of the dependencies.
 --

I'm unsure if this is the way to go. I'll see if I can find any issues with it.

@martijnbastiaan
Copy link
Contributor

At the risk of spamming.. you can test this patch by:

git remote add martijnbastiaan https://github.com/martijnbastiaan/tasty.git
git fetch martijnbastiaan
git cherry-pick 0d2dee1341a94ce22d17f12165528e8975c88bce
git remote remove martijnbastiaan

@UnkindPartition
Copy link
Owner Author

Thanks Martijn, the fix wasn't quite right but your diagnosis saved me a lot of time. This should be fixed now.

@martijnbastiaan
Copy link
Contributor

Neat, thanks Roman! I've successfully implemented this feature in the Clash testsuite, and it works really well. Looking forward to the next Tasty release :-).

UnkindPartition added a commit that referenced this issue Nov 11, 2018
See #48

Thanks to Ryan Scott and Martijn Bastiaan for testing earlier
versions of this feature.
@UnkindPartition
Copy link
Owner Author

Dependencies are now documented in the README and merged into master.

I'll wait for another week or two for any remaining feedback/bug reports and then make a release.

@martijnbastiaan
Copy link
Contributor

Thanks, looking forward to the new release! I just wanted to give you an overview of how I incorporated the dependent tests in the Clash testsuite. First a little bit of setup: Clash is a compiler, so our tests typically look like:

  1. Running the compiler for target $ARCH
  2. Compile generated files with appropriate compiler (ghdl, verilator, ..)
  3. Run compiled files against test cases

If any of these steps fail, the next one should not be run. It's also a lot of work to write each of these steps for every specific test case, so we wrote a function runTest to generate these:

runTest :: ..... -> TestTree

We group our tests using testGroup, a little bit like:

testGroup "AllTests" $ [testGroup "A" [runTest x, runTest y]] 

For runTest to generate correct test patterns it should have access to the fully qualified name of the test cases it is generating. So we rewrite to:

clashTestRoot "AllTests" $ [clashTestGroup "A" [runTest x, runTest y]] 

with:

-- | Same as `clashTestGroup`, but used at test tree root
clashTestRoot
  :: [[TestName] -> TestTree]
  -> TestTree
clashTestRoot testTrees =
  clashTestGroup "Tests" testTrees []

-- | `clashTestGroup` and `clashTestRoot` make sure that each test knows its
-- fully qualified test name at construction time. This is used to create
-- dependency patterns.
clashTestGroup
  :: TestName
  -> [[TestName] -> TestTree]
  -> ([TestName] -> TestTree)
clashTestGroup testName testTrees =
  \parentNames ->
    testGroup testName $
      zipWith ($) testTrees (repeat (testName : parentNames))

and

runTest :: ..... -> [TestName] -> TestTree

So now runTest will get the names of its parents (in reverse order), so it can generate a pattern like:

$0 == "AllTests.A.Architecture1"

which I've implemented as:

-- | Given a number of test trees, make sure each one of them is executed
-- one after the other. To prevent naming collisions, parent group names can
-- be included.
sequenceTests
  :: [TestName]
  -- ^ Parent group names
  -> [(TestName, TestTree)]
  -- ^ Tests to sequence
  -> [TestTree]
sequenceTests path (unzip -> (testNames, testTrees)) =
  zipWith applyAfter testPatterns testTrees
    where
      -- Make pattern for a single test
      pattern :: TestName -> String
      pattern nm = "$0 == \"" ++ intercalate "." (reverse (nm:path)) ++ "\""

      -- Test patterns for all given tests such that each executes sequentially
      testPatterns = init (Nothing : map Just testNames)

      -- | Execute a test given as TestTree after test given as FQN
      applyAfter :: Maybe String -> TestTree -> TestTree
      applyAfter Nothing  tt = tt
      applyAfter (Just p) tt = after AllSucceed (pattern p) tt

I hope this gave some insight.

@UnkindPartition
Copy link
Owner Author

Ok, interesting.

One thing I'd suggest is to construct the pattern AST directly instead of assembling a string pattern. You can see an example of that here: https://github.com/feuerbach/tasty/blob/master/benchmarks/dependencies/test.hs#L37-L38

@martijnbastiaan
Copy link
Contributor

Right, that's better. Thanks 👍 .

@UnkindPartition
Copy link
Owner Author

This is now released as part of tasty-1.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests