Add TaskSeq.takeWhile, takeWhileAsync, takeWhileInclusive, takeWhileInclusiveAsync

[bartelink]

Fixes: #122 (edit: see also #235)

• Impls
• Basic test for takeWhile
• Prove items after test are not iterated
• Extend tests to cover takeWhileInclusive
• Cover takeWhileInclusive(Async)? wrt Immutability/Side effects ?

[bartelink]

Remaining question is how/whether to cover the Immutability/Side-effecting aspects for the Inclusive variants
Could add a comment to the impl saying "if you make the impl vary from the non-inclusive variant, first get the tests to cover it"

Seriously though, rather than duplicating L99-135 with s/While/WhileInclusive and tweaked expectations, I assume the best thing is to change module Immutable and module SideEffects to be an [<AbstractClass>] type and then provide two concrete variants with inclusive being true/false respectively (in order to cover the combination of variance of test cases vs the variation of the inclusive/exclusive).

Don't really have other ideas as to how to cover both variants beyond either
1.running the two functions successively within the same Test Method
2. extracting the bodies of the tests into helper functions and copy-pasting the modules / functions / Theory stuff

[abelbraaksma]

Thanks for this!

What I usually do for testing side-effects where the "run all elements" doesn't suffice, is to just add something like:

[<Fact>]
let myTest() =
   let mutable x = 0
   let ts = taskSeq {
        x <- x + 1
        yield x
        x <- x + 1  // ensure we don't execute this
        yield x
   }
   let res = TaskSeq.takeWhileInclusive (fun _ -> false)
   x |> should equal 1
   res |> TaskSeq.exactlyOne |> should equal 1
}

And:

[<Fact>]
let myTest() =
   let mutable x = 0
   let ts = taskSeq {
        x <- x + 1
        yield x
        x <- x + 1
        yield x
   }
   let res = TaskSeq.takeWhile(fun _ -> false)
   x |> should be 1
   res |> TaskSeq.isEmpty |> should be True
}

[abelbraaksma]

Great work, I like it that you're following the same patterns that were there. Thanks for this. See my comments below. Nothing major:

[abelbraaksma]

One more nit:

[bartelink]

I have not looked at the rest of the test codebase enough to make a full call on this so would appreciate your candid response (tbh the FsUnit style stuff is not something I'm well versed in - I tend to use unquote and computation expressions only for my tests)...

I could revisit the tests to do side-effecting of counters/markers and validating the presence/absence of same in line with your suggestion instead of having the exceptions acting as guards.

Happy to do a reorg to follow house style (or only final tweaks based on staying with the throwing) based on your call.

[abelbraaksma]

I'm ok with everything so far, I can add the mutability tests (just before/after what's supposed to be yielded) myself as well. The failwith approach is fine for now, it can certainly stay.

A simple example of what I mean can be found in the SideEffects section of TaskSeq.Head.Tests.fs, for instance. It is quite straightforward, actually.

House style: I think you pretty much nailed that one already! :) And some variation in style w.r.t. tests is never problematic.

Using unquote: this code relies heavily on resumable code, valuetasks, tasks and asyncs, and unless it is fixed now, Unquote, or quotations in general, have some issues with that. I haven't dived deep into it. Probably good to at least add a test to find out what's going on or how far it is supported. But that's a subject for another time.

[bartelink]

Good suggestions; added tests that I hope cover the intent sufficiently.
They're starting to smell of too much logic in the tests, but this way does give decent coverage of the async * internal matrix without large swathes that no human can actively read...

Happy to tweak re any comments you may have, but equally happy if you want to express it as a commit and/or fix it afterwards.

---

Using unquote: this code relies heavily on resumable code, valuetasks, tasks and asyncs, and unless it is fixed now, Unquote, or quotations in general, have some issues with that. I haven't dived deep into it. Probably good to at least add a test to find out what's going on or how far it is supported. But that's a subject for another time.

I was referring more to the use of unquote to express the assertions, in lieu of the matcher stuff. In general I'd have any use of Task/Async and such elements confined to the Act phase so I believe your main concern would not arise. Examples of the benefits of that would be

• not having to discover not'
• being able to rattle off assertions such as test <@ repeat |> Array.forAll (fun x -> x < 12) @> without having to go chasing around to discover how to express that in the DSL

In short, I believe Unquote's assertions provide a balance of properties that would suit a project like this:-

• people do not need to ramp up on an assertion DSL (be that any of NUnit's flavours, xUnit, Expecto or FsUnit) and can thus read and write them naturally (it also makes them neutral to the test host)
• there's pretty good failure messages out of the box, though it's definitely possible to make a mess if your expression is overly complex
• while it arguably is not as friendly for F# beginners when compared to reading FsUnit, I'd argue that it's a good teaching tool for people to be forced to expand their usage of normal F# expressions to express assertions in a legible manner (though that does require more implicit judgement than being guided by the contours of a DSL's structure)

Articles:

• Good TL;DR summary https://stackoverflow.com/a/5669263/11635
• Another article: https://blog.ploeh.dk/2014/03/21/composed-assertions-with-unquote/

I appreciate that having >1 assertion DSLs in a project is not a good idea, so definitely another day's discussion

[abelbraaksma]

I've used Unquote with test <@ ... @> in almost all projects I've worked with, I'm well aware of the benefits :). I also like FsUnit's explicitness, but this comes with the downsides you mention. Since most tutorials mentioned (or used to mention?) FsUnit, I often consider it the preferred choice for easy adoption to others. Perhaps that's not the case anymore.

(rambling: this is a high-end library with lots of cutting edge code that only few programmers will be able to actually modify/maintain, notably the CE stuff, so using a low-end lib for testing doesn't really add anything: people working on this will have been around the block)

Since we're slowly moving out of the prototype phase, I may actually rewrite tests in Unquote at some point. Whether it'll really be more concise in the long run, I don't know.

FYI: Currently, running all tests in CI takes some 10 min. Locally around 1 min (in parallel). This is in part because there are deliberate delays to force the "thread yield" behavior. Otherwise, the tests wouldn't really test any thread-switching and all task stuff would just run synchronously. Also, there are a few rudimentary perf tests littered throughout (they should go in a separate benchmark project).

Appreciate your thoughts, always good to share insights. I'll have a moment later today to go over your changes, thanks!

[bartelink]

I've used Unquote with test <@ ... @> in almost all projects I've worked with, I'm well aware of the benefits :). I also like FsUnit's explicitness, but this comes with the downsides you mention. Since most tutorials mentioned (or used to mention?) FsUnit, I often consider it the preferred choice for easy adoption to others. Perhaps that's not the case anymore.

Yes, saw a thread in the FSSF Slack immediately after that makes that abundantly clear, sorry for getting the wrong end of the stick

I would not expect unquote to make the assertions all that more concise.

people working on this will have been around the block

True (though I tend to sidestep using stuff other than xunit and unquote)

I personally have not run into FsUnit for ages, but it really does depend on where you look. There's definitely plenty higher value things than esp you can do than porting assertions to unquote.

If you make a task for it, you might tempt me on one of these dark winter nights, but again: I doubt it's a true deal breaker for contributions to this repo. However I won't bring it up again unless you signal desire by doing that ;)

[abelbraaksma]

Excellent work, thanks for this! I'll make a few final tweaks, see my comments below, and merge it in. Hope that's ok with you (that I hijack your PR a little bit).

[abelbraaksma]

@bartelink I made a few changes, see above. If you are ok with these, I'll go ahead and merge it in.

[bartelink]

Changes look great, go for the merge (though note the MemberData suggestion)

[abelbraaksma]

though note the MemberData suggestion

Yep, agreed. Will be part of a revamp of the tests in general.

[abelbraaksma]

@bartelink thanks for all your work and patience with this, it is much appreciated! Will be part of the next version of TaskSeq (0.3.x or 0.4.0), probably in a few days.

[bartelink]

Thanks! No rush of any kind on the release (but I'll definitely use it when it hits nuget)