WIP: Slimmed-down expecto runner

[baronfel]

I thought I might take a stab at removing some of the reflection magic and making the koans more simple using expecto. I've started the process of translating some of the tests so that we'd have a realistic-ish idea of what such a transformation might look like.

I was able to remove the two support projects, using only a light two-file runner wrapper that delegates to expecto to actually run the tests.

The tests are defined per-module as before, we just aggregate together the test lists in the PathToEnlightenment.fs file.

I've also added some files that enable CMD+B and F5 build and debugging for the project, so taht cross-platform users have a nicer onboarding session.

I also switched the package management over to paket, which is a completely not-user-impacting change, but lets us more easily do the restore as part of the build. That minimizes the number of commands a new user should have to run.

I've taken care to keep the output as similar as possible too.

Do you think there's value in this?

An example of the output from a run is:

chet@Chet-MacBook ~/code/oss/FSharpKoans
expecto* $ mono --debug FSharpKoans/bin/Debug/FSharpKoans.exe                [17:49:37]
teaching about assertions:
	'fill in values' passed
	'assert expectation' passed


teaching about let bindings:
	'you cannot modify a let bound value if it is not mutable' failed


You have not yet reached enlightenment ...
fill in the __

Please meditate on the following code:
  at FSharpKoans.about [email protected] (Microsoft.FSharp.Core.Unit unitVar0) [0x00007] in /XXX/FSharpKoans/FSharpKoans/AboutLet.fs:82
  at [email protected] (Microsoft.FSharp.Core.Unit unitVar) [0x00018] in <590315ee0c1a9511a7450383ee150359>:0
  at Microsoft.FSharp.Control.AsyncBuilderImpl+callA@839[b,a].Invoke (Microsoft.FSharp.Control.AsyncParams`1[T] args) [0x00052] in <5939249c904cf4daa74503839c243959>:0

It's important to have the --debug on the mono invocation to get the line number on the stack trace :)

[ChrisMarinos]

I haven't forgotten this; just need to make some time to pull it down and take a look. Overall, I do like the idea of a slimmed-down runner, and have always preferred the idea of keeping the koans as simple as possible in terms of potentially-confusing syntax like the Koan attribute. It's been a while, but there was an original version of the koan runner that had some of those features before we replaced it with the current version (written by Ryan Riley). That version had some other downsides, though, so hopefully this ends up being the best of both worlds!

[baronfel]

Ok, I finally got around to finishing the port of these tests over to expecto. I'd also like to take a look at a conversion to the new SDK/.net core, as I've done that several times already.

The big win there is using dotnet watch to automatically run the tests as the user fixes them up!

[vilinski]

@baronfel how do you the conversion to new SDK/.net core? Is the some automatization or plain manual work? Just curious.

[baronfel]

It's really simple for simple projects like these, especially thanks to paket. The final fsproj will look like something very close to:

<Project Sdk="FSharp.NET.Sdk;Microsoft.NET.Sdk">
  <PropertyGroup>
    <AssemblyName>FSharpKoans</AssemblyName>
    <OutputType>Exe</OutputType>
    <TargetFrameworks>net45,netstandard1.6</TargetFrameworks>
    <DebugType>pdbonly</DebugType>
  </PropertyGroup>
  <ItemGroup>
    <Compile Include="Asserts.fs" />
    <Compile Include="AboutAsserts.fs" />
    <Compile Include="AboutLet.fs" />
    <Compile Include="AboutFunctions.fs" />
    <Compile Include="AboutTheOrderOfEvaluation.fs" />
    <Compile Include="AboutUnit.fs" />
    <Compile Include="AboutTuples.fs" />
    <Compile Include="AboutStrings.fs" />
    <Compile Include="AboutBranching.fs" />
    <Compile Include="AboutLists.fs" />
    <Compile Include="AboutPipelining.fs" />
    <Compile Include="AboutArrays.fs" />
    <Compile Include="AboutLooping.fs" />
    <Compile Include="MoreAboutFunctions.fs" />
    <Compile Include="AboutDotNetCollections.fs" />
    <Compile Include="AboutTheStockExample.fs" />
    <Compile Include="AboutRecordTypes.fs" />
    <Compile Include="AboutOptionTypes.fs" />
    <Compile Include="AboutDiscriminatedUnions.fs" />
    <Compile Include="AboutModules.fs" />
    <Compile Include="AboutClasses.fs" />
    <Compile Include="AboutFiltering.fs" />
    <Compile Include="PathToEnlightenment.fs" />
    <None Include="App.config">
      <CopyToOutputDirectory>Always</CopyToOutputDirectory>
    </None>
  </ItemGroup>
  <Import Project="..\.paket\Paket.Restore.targets" />
</Project>

so basically a glorified list of files. Paket handles adding the correct PackageReference nodes. Anything past this I'm usually using FAKE for, to coordinate output directories and all that, so my project files don't usually get too bad

[baronfel]

The other way to do it would be to run dotnet new console/library/whatever --lang f# to get the base template and then add the ItemGroup for the files you need.

[vilinski]

Thanks. So it's basically how I've done it before with a few of my hobby projects - dotnet new mini-scaffold ... and adding old files.

[ChrisMarinos]

Sorry that I've been so bad about replying to this sooner. I did test this branch out, and I noticed that all of the tests seem to run in reverse. For example, in AboutAsserts, the "fill in values" koan executes before "assert expectation".

I'd also like to find a way to clean up the syntax of the koans a little bit. I think it's great that this removes the need for the [<Koan>] attribute, but (and this is entirely a matter of taste) I think the new syntax is a little less-friendly to someone who is brand-new to F#. Consider the first "about asserts" module:

[<Koan(Sort = 1)>]
module ``about asserts`` =

    [<Koan>]
    let AssertExpectation() =
        let expected_value = 1 + 1
        let actual_value = __ //start by changing this line
     
        AssertEquality expected_value actual_value
 
    //Easy, right? Now try one more

    [<Koan>]
    let FillInValues() =
        AssertEquality (1 + 1) __

With the current syntax, the reader has to parse the [<Koan>] attributes, the module keyword, various let statements, AssertEquality, and the __ placeholder. That's a lot to absorb, but I think the "koan framework" elements here are minimal. The big lift is the [<Koan>] attribute, but I think that an attribute, by nature, is visually distinct from the rest of the code, and thus more easy for a newcomer to ignore. If they're coming from a language with attributes, they also may recognize that this is an attribute and know to ignore it for the time being. Finally, by being called "koan", the attribute serves as a visual indicator of separation between each lesson.

Now for the Expecto syntax:

module ``about asserts`` =

  let tests =
    testList "teaching about assertions" [
      testCase "assert expectation" <| fun () ->
        let expected_value = 1 + 1
        let actual_value = __ //start by changing this line

        AssertEquality expected_value actual_value

      //Easy, right? Now try one more
      testCase "fill in values" <| fun () ->
        AssertEquality (1 + 1) __
    ]

Here the reader no longer needs to parse the [<Koan>] attribute- which is great! However, I think that gain comes at the sacrifice of some other "scary" looking syntax and visible "koan framework" elements. In addition to the elements of the previous syntax, a newcomer is also presented with testList, testCase, a assignment to the tests variable, <|, [ ], ->, and the fun keyword. They also lose a strong visual indicator of where each lesson begins. To remedy this, I think that one starting point may be to alias testCase and testList as something like koan and koanList.

However, I think that it would be ideal to find a syntax that could also hide some of the other "koan framework" elements and "advanced" F# syntax. One of the pieces of feedback that I often received from teaching F# to newcomers was that they were overwhelmed by the number of symbols used in F#, and I think that this syntax might be less approachable than the current syntax for those folks.

On the other hand, I really like how this PR massively simplifies the overall solution, eliminates the need for the Core and Test projects, and makes it dead-simple about what project to build and run. Overall, great work for simplifying those things, and let me know what you think about the rest of my feedback.

[baronfel]

I don't think it'd be hard to go back to a simpler attribute-style way of working, honestly. Expecto helps here by encapsulating the work of scraping the current assembly for tests, so most of the work would go into making sure the order is correct. If those thoughts prove to be true, then we should get the best of both worlds: simpler test syntax and simpler runner infrastructure.

[ChrisMarinos]

Yeah, I definitely think that's one route forward. I just didn't want to dismiss all the progress you made/work you did without explaining my thought process. I'm totally open to suggestions if you have other thoughts, though.

[baronfel]

how does this look to you:

module ``about asserts`` =

  let tests =
    koans "about asserts" [
      koan "assert expectation" {
        let expected_value = 1 + 1
        let actual_value = __ //start by changing this line

        AssertEquality expected_value actual_value
      }

      koan "fill in values" {
        AssertEquality (1 + 1) __
      }
    ]

Attributes are a little odd with expecto, because you'd end up with something like this:

module ``about asserts`` =
  [<Koan>]
  let test1 =
      koan "assert expectation" <| fun () -> 
        let expected_value = 1 + 1
        let actual_value = __ //start by changing this line

        AssertEquality expected_value actual_value

  [<Koan>]
  let test2 = 
      koan "fill in values" <| fun () -> 
        AssertEquality (1 + 1) __
    

or something very much like that with a koanList [] function. But I think the koan Computation Expression (renamed from the test computation expression expecto ships with) provides a pretty natural syntax.

[baronfel]

a little bit of regex magic later and we're back to good. Now I've got the ordering corrected again, too. F# Linked list order sometimes flips depending on the processing...

[ChrisMarinos]

I like this syntax much better, though it'd be nice if we could find a way to clean up the syntax at the top of the file a little bit:

module ``about asserts`` =

  let tests =
    koans "about asserts" [

Ideally it would just be just one line- something like koans "about asserts" [.

The only other concern that I have with this new-and-improved-syntax is that it makes me a little nervous to run everything inside a computation expression. I can't think of any concrete reasons for this, but I'm just going to take a quick run through of the koans myself to make sure that nothing funny/confusing happens when running them inside a computation expression.

[baronfel]

The only one that seemed to do anything different was the looping with while construct one, because the test builder CE doesn't implement while. I was going to open a PR over on expecto for that.

But really the CE is very lightweight here and just replaces the function pipe form of the test case function.

I totally agree on the duplication at the top of the modules, I'm wracking my brain trying to figure out how to reduce that....

[ChrisMarinos]

I noticed a couple other of things from running them.

1. The exception messages are a little less descriptive. I'm not sure how expecto works, but I think it's important to have a message stating:

You have not yet reached enlightenment ... Expected: 2 But was: 1

2. I also noticed that I occasionally don't get a stack trace from a failure. I've noticed this behavior on on the last AboutAsserts, and the last AboutFunctions koans, but it seems that I can't repro it consistently. I'll let you know if I can pin it down.

[kunjee17]

@baronfel hey, I found a issue while trying this. In About stock example you will required to put test case inside test list. Else toGroupName will failed.

And also need to List.Rev all testlist so they will appear in correct order.

And in AboutDotNetCollection.fs we are compering two Seq. That is not possible. So, code should have either list or array.

testCase "skipping elements" <| fun () ->
          let original = [0..5]
          let result = List.skip 2 original

          AssertEquality result [2..5]

Thanks for putting it on dotnet core. Make it easier for people who don't have or don't want to install VS.

[kunjee17]

@baronfel Same test list issue there in OpenMoudule test case. List is required there too.