Migrate Cambridge test suite (tests/fsharp) runner to NUnit

[enricosada]

This is a spike to explore replacing current .bat based test runner with NUnit, for Cambridge suite ( tests/fsharp ).

There is a discussion on codeplex about the Redmond suite ( tests/fsharpqa ), the idea is the same, Cambridge is easier to port before attempt Redmond suite

Current .bat based runner has some problems:

• windows only
• no integration with ide test runner
• test run flow and rules ( 'dont.use.empty.signature' files, config, env vars ) are in multiple files and not documented

New runner

• tests are not modified, a new runner is added side by side to current
• in library proj -> tests\fsharp\FSharp.Tests.fsproj
• group by modules and tag with NUnit categories
• core\access -> module FSharp-Tests-Core.Access inside tests/fsharp/core/_test.fs
• generate nunit test with TestCaseSource and alike
• add categories programmatically to generated test
• convert .bat files to fsharp ( single-test-build.bat -> single-test-build.fs ), easier to convert and diff

This pr when complete

1. smoke test fsi/fsc before run tests (failfast)
2. improve command log (log every command, path and arguments)
3. run tests inside ide ( [X] vs, [ ] xamarin, [ ] resharper )
4. cross platform
5. filter tests by category
   • by directory ( like core, core/events )
   • by tags inside .fs files ( like #Records in tests\fsharp\core\access\test.fsx )
   • failed previous run
   • custom tag with NUnit CategoryAttribute
6. run tests from command line using nunit test runner
7. run tests from command line using tests\RunTests.cmd

I need some feedback

• usefull?
• how to improve readability of code ( line by line .bat -> .fs ) for easier port, like in tests\config.fs ref/mutable mess
• how to remove workaround for ITestAction.Targets property (see tests/fsharp/nunitConf.fs:114 )
• some 👀 to discover opportunity of cleanup
  • already converted nested match Ok -> | ErrorLevel -> to computation expression
• missing features of old runner?

workflow

1. implement new runner without changing tests

2. put old .bat code inside test for easier diff, line by line

   // "%FSC%" %fsc_flags% -a -o:test.dll -g test.fs
   do! fsc "%s -a -o:test.dll -g" cfg.fsc_flags ["test.fs"]

3. run side by side with old runner

4. modify .bat suite runner ( fsharp\tests\RunTests.cmd ) to use nunit test runner for Cambridge suite only

5. remove .bat files of test runner

Future work (not this pr, but easier to implement after)

1. replace file based test control (like dont.use.empty.signature file) with code or NUnit categories
2. replace tags inside files (like #Records) with NUnit categories
3. replace file based test result ( like test.ok, build.ok ) with exit code only or function result
4. cleanup now useless workflow ( build.bat -> build.ok file )
5. run test in parallel
6. run multiple configuration ( net40, portable ) in parallel

Notes

VS Test Runner

To filter test inside visual studio test runner:

• Project:FSharp.Tests to show only Cambrige suite
• Trait:FSI_FILE to filter only FSI_FILE category (see FSharpTestSuiteTypes.Permutation union )
• Trait:access to filter 'core/access'
• -Trait:FSC_HW to exclude FSC_HW tests

ex: Project:FSharp.Tests Trait:FSC -Trait:FSC_HW run all tests with FSC tag but not FSC_HW

[TEST CHANGED]

I added changed test in separate commits with prefix [TEST CHANGED] because found bug/problems.
These can be evaluated before this pr, and easier to review/cherry-pick

[forki]

How does this relate to #15?

[dsyme]

Impressive. I'd dearly love to get rid of all the .bat rubbish (originally my fault!!!) and replace it by F# scripting and NUnit.

Somewhat ortogonally, I've often wondered I do wonder if large parts of the fsharp/... test suite that is "single-test-and-run" could simply be moved into FSharp.Core.Unittests.dll. Much of it is just testing functional correctness of the F# core library and F# core language constructs. We reused much of the suite to somewhat randomly test things like localization and signature file generation, and always drove it through the "fsc.exe" tool.

[enricosada]

@forki is orthogonal because

1. the fake build script doesn't run tests/fsharp and tests/fsharpqa test suites
2. is only a nunit test library, like current unit tests in FSharp.Core.Unittests project
3. if you add these suites to fake as task, when this pr is merged instead of run tests/RunTests.cmd, you can use Nunit task to run tests\fsharp suite

if your pr is merged before that one, i'll add the task for run it instead of modify RunTests.cmd

the only problem i see, is we both replaced src/update.cmd with f# code ( mine is src/update.fs ), but i think i can simply call fake and run update task

[forki]

@enricosada very cool. we should now try to keep both PRs compatile. 👍

[enricosada]

@dsyme Absolutely, there are lot of possible cleanup after the rewrite, with type safety now 💃 and easier to spot 👀

example 1: 'core/access'

module Access =
    let permutations =
        FSharpTestSuite.allPermutation
        |> List.map (fun p -> (new TestCaseData (p)).SetCategory(sprintf "%A" p) |> setTestDataInfo "access")

    [<Test; TestCaseSource("permutations")>]
    let access p = check (processor {
        let { Directory = dir; Config = cfg } = testContext ()

        do! SingleTestBuild.singleTestBuild cfg dir p

        do! SingleTestRun.singleTestRun cfg dir p
        })

This use only single-build -> single-run

easy to spot core/access use only:

• fsc
• fsi
• spanish/french are useless, because inside 'tests.fsx' the thread culture is not changed

⭐ a good candidate to move to unit tests.

example 2: core/quotes

the implementation (see '_tests.fs') after an easy and boring refactor is.

module Quotes = 

    [<Setup>]
    let quotes () = check (processor {
        do! csc """/nologo  /target:library /out:cslib.dll""" ["cslib.cs"]
    })

    [<Test>]
    let `test fsi` cfg dir = check (processor {
        do! fsi "%s -r cslib.dll" cfg.fsi_flags ["test.fsx"]
     })

    [<Test>]
     let `test fsc` cfg dir = check (processor {
        do! fsc "%s -o:test.exe -r cslib.dll -g" fsc_flags ["test.fsx"]
        do! peverify "test.exe"

        do! clix ("."/"test.exe") ""
    })

    [<Test>]
     let `test fsc --optimize` = check (processor {
        do! fsc "%s --optimize -o:test--optimize.exe -r cslib.dll -g" fsc_flags ["test.fsx"]
        do! peverify "test--optimize.exe"

        do! clix ("."/"test--optimize.exe") ""
    })

Maybe can start a discussion:

• do that three times ( fsi/fsc/fsc optimized ) is usefull?
• just include 'tests/fsharp/core/quotes/test.fsx' as 'unit tests\quotes.fs' and call directly the functions

⭐ a good candidate to move to unit tests.

[dsyme]

Seeing all those test variations is very cool. It shows that with this PR we could know more easily all the variations and tests that we're actually running :)

[enricosada]

yes, i added also reason why the test is skipped:

a future cleanup is to remove the test case, instead of use file ( es dont.pipe.to.stdin ) flow control

[latkin]

I haven't taken a careful look, but it's awesome to see some stuff getting bootstrapped here.

I do have some (rather, very many) thoughts on how to effectively re-architect the tests, will try to distill those and share sometime over the next few days.

Independent of this particular PR, @KevinRansom and I are trying to figure out the best way to handle these "WIP" kinds of PRs, which are very valuable, but will likely required extended iteration and design changes. Managing these on a separate fork might be the best approach.

[forki]

"already"

[jack-pappas]

@enricosada This is great work, and is exactly what I had in mind in our earlier CodePlex discussion about implementing a new test runner.

A few ideas/comments/suggestions:

• You've added a "cross platform" work item for this PR. I'd like to see that too, but the current unit tests aren't cross-platform, so in the interest of getting this merged in, cross-platform support is something we should tackle afterwards as a separate improvement.
• Regarding the other two outstanding work items related to running the tests from the command-line -- why do we need both of them? If we were able to run the tests using the NUnit console runner, why would we still need to keep RunTests.cmd around?

@latkin Can you share your thoughts on re-architecting the tests?

[enricosada]

I'll push all tests on this branch today (+ lot of cleanup ).

Thx @jack-pappas, i got the idea from your post on codeplex

Last todo is to use a state monad to manage current directory+env vars, but i think is ok like this, a bit verbose but work ok ( i need an update monad for current directory and env var -> awesome )

About RunTests.cmd, compability is for ci build, the RunTests.cmd will call the nunit-console
For xplat i think almost all tests are ready, but we can fix them later to speedup merge.

About xplat:
i used a config type to refactor out almost all windows logic ( path, etc), so a tipical test is like

[<Test; TestCaseSource("testData")>]
let hiding () = check (processor {
    let { Directory = dir; Config = cfg } = testContext ()

    let exec p = Command.exec dir cfg.EnvironmentVariables { Output = Inherit; Input = None; } p >> checkResult
    let fsc = Printf.ksprintf (Commands.fsc exec cfg.FSC)
    let peverify = Commands.peverify exec cfg.PEVERIFY
    let fsc_flags = cfg.fsc_flags

    // "%FSC%" %fsc_flags% -a --optimize -o:lib.dll lib.mli lib.ml libv.ml
    do! fsc "%s -a --optimize -o:lib.dll" fsc_flags ["lib.mli";"lib.ml";"libv.ml"]

    // "%PEVERIFY%" lib.dll
    do! peverify "lib.dll"

    // "%FSC%" %fsc_flags% -a --optimize -r:lib.dll -o:lib2.dll lib2.mli lib2.ml lib3.ml
    do! fsc "%s -a --optimize -r:lib.dll -o:lib2.dll" fsc_flags ["lib2.mli";"lib2.ml";"lib3.ml"]

    // "%PEVERIFY%" lib2.dll
    do! peverify "lib2.dll"

    // "%FSC%" %fsc_flags% --optimize -r:lib.dll -r:lib2.dll -o:client.exe client.ml
    do! fsc "%s --optimize -r:lib.dll -r:lib2.dll -o:client.exe" fsc_flags ["client.ml"]

    // "%PEVERIFY%" client.exe
    do! peverify "client.exe"

    })

we can run same test on mono (minus ngen)

for paths i fixed all directory separator (using alwasy Path.Combine) like

  do! exec ("."/"test--optimize.exe") ""

or

  do! exec (".."/"ConsoleApplication1"/"bin"/"Debug"/"Profile259"/"PortableTestEntry.exe") ""

[enricosada]

ok, pretty much done 😄 , all tests migrated.

Skipped tests

these are ok, not run because rules like:

• file dont.use.empty.signature exists
• file test.ml doesn't exists

see test output message, about reason

Failed tests

I fixed test where possible ( see [TEST CHANGED] commits )
About the others:

test (testcase fail/total)	with old runner	reason
core/load-script (1/1)	pass	output diff
core/members/basics (3/17, 14 ok)	pass	i can repro with command line, see comment on test code
core/pinvoke (1/1)	pass	the test pass, but i dont know about ILX_CONFIG env var, if not defined or blank the test pass
core/printing (4/5, 1 ok)	pass	fail on output diff, some problems with my current culture
core/queriesOverOData (1/1)	flacky	download data from internet, fail if error during download
regression/656 (1/1)	pass	the test pass, but i dont know about ILX_CONFIG env var, if not defined or blank the test pass
typeProviders/oDataService (11/17, 6 skipped)	flacky	download data from internet, fail if error during download
typeProviders/wsdlService (11/17, 6 skipped)	flacky	download data from internet, fail if error during download
typeProviders/diamondAssembly (1/1)	false positive	fixed test with commit, was a false positive
typeProviders/negTest (2/28, 26 ok)	fail	fail on case providerAttributeErrorConsume, ProviderAttribute_EmptyConsume

i need some info:

• environment variable ILX_CONFIG: where is configured? i dont see it on repo or visual studio command prompt. if blank, tests pass
• flacky test: test pass if download is possibile, but imho must be changed (for example wsdl can be included in repo)
• diff file: some errors are because of my regional settings (italian), but others not

run tests

You should try in visual studio, using visual studio test runner (nunit adapter required)

Now test fixture read file tests\test.lst, so you can do Project:FSharp.Tests Trait:Smoke to run smoke tests

For other filters options see previous post

I tagged failing test on my machine with category:

• fail_new , fail with nunit runner
• fail_old, fail with old runner

you can easy filter them with Project:FSharp.Tests Trait:fail_new

notes

Whole test suite is 3h on my machine (all permutations).
With commit 7df7694 i configured to run all test and all permutation (like FSI_FILE, FSC_BASIC etc).
There was a random 😨 on test list, so old runner only run a subset

Some test are useless ( see previous post ), we can cleanup unused permutations.

About code formatting style, there is a style guide for this repo? spaces, etc.
I want to use automatic and standard formatting with fantomas (Ctrl-K Ctrl-D FTW), but now is difficult to review line by line with old .bat code. Maybe later after review and cleanup of comments

can you @dsyme and @latkin try it?

[enricosada]

or @KevinRansom too, sry I forgot to ask you too (is a good idea to create a github group? @fsharp/visualfsharp or @Microsoft/visualfsharp )?

I want to start with fsharpqa if this is ok (or need improvement but is ok as a way to migrate)

[KevinRansom]

Enrico,
It's been two months and it seems like you made a great start. How do you want to proceed with this?

[enricosada]

I waited for some reviews, but this pr is done, i removed the [WIP]

open points:

crossplatform

The test runner is ok.
I can fix single tests compatibility on openfsharp repo after merge, like @jack-pappas said

use new runner instead of old one

change tests\RunTests.cmd to use new runner, i dunno if you want to do that, but i hope so we can remove useless .bat files

nunit compatibility

@latkin said TestCaseSourceAttribute cannot be used on this repo because your internal build server use an old nunit version (2.2 if i remember right).
we use 2.6.4 in this repo
This is an additional project so build is not a problem
We can run nunit from command line instead of the current test runner, just ask

let me know, i'll rebase soon to remove build failure (does not depend on this pr)

[jack-pappas]

@enricosada Can this new runner live alongside the old one (i.e., the *.bat and *.pl scripts)? If so, removing the old runner is not a prerequisite to merge this in -- we can keep them both for the immediate future and start removing obsoleted pieces of the old system in separate PRs.

@latkin @KevinRansom Is there any reason not to merge this in now?

[enricosada]

sure, this code add only code, doesnt change the current test suite
i have not changed .bat files or old runner

as @jack-pappas said we can remove the old code later (hopefully soon ) .

About ms internal build server, the build script download the nunit test runner, so is easier to run the suite from the current bat file

If needed i can modify the bat runner to use nunit if a environment variable if set (like TESTSUITE_FSHARP_USE_NUNIT ) so the runner can run both ( easier to compare )

[msftclas]

@enricosada, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

[enricosada]

i need to change nunit -> xnunit? /cc @latkin @KevinRansom

[KevinRansom]

You don't need to change nunit to xunit. Although we may well change the repo over to repo at some point in the future.

The way to think about it is this:

• the code that is checked in is the state of the universe
• Issues are expressions of hope for a better future

To cut a short story long, we may or may not do the things we have as issues, doing so depends on many things, not least of which is that more than one person buy of on a goal.

I hope that clears things up somewhat.

[enricosada]

ok @KevinRansom but what are current goals? excluded bugfixs for fsharp4.
I should wait until release of vs2015 for additional work on feature/improvment?

[latkin]

This PR is really nice work, and a good starting point for any future contributions aimed at refactoring the tests. Unfortunately, it has been around for quite some time and will not realistically be merged, so I am finally closing it out.

Overall, my take on it is that this PR is essentially just translating the current fsharp tests and test infrastructure directly from batch to F#. That's well and good -- more powerful/maintainable language, some nicer ways to execute tests and see results -- but doesn't do anything to address the real problems with the test system. This large of a change is not justified if we are left with most of the same core issues as before.

The deeper issues I see with the current system (and what a full refactoring should accomplish) include

• Still isolated/different from fsharpqa tests
  • All language-level compiler tests should be in the same suite, implemented with the same overall patterns
• fsharp tests are a wild-west of ad hoc scripts and tools
  • Related to above item. Some use the single-test-run pattern, which is good, but many don't. This makes it very tedious to do large-scale work with these tests, as each needs to be considered uniquely.
  • As much as possible these tests should be refactored to use the same simple patterns. Any that can't probably cover high-level user scenarios and should be taken out and placed in some separate suite.
• Not portable/cross-platform compatible
  • Core test engine should be portable
  • All platform-agnostic tests should be together
  • Any Windows-specific tests (e.g. needs ngen) should be together, separate from pure language-level tests
  • Basically, tests and test engine needs to be properly factored/layered based on platform dependencies, not all balled together
• Not flexible to different variations
  • fsharp suite supports various "permutations" but has no easy way to run tests targeted to a different platform. fsharpqa suite is opposite.
  • We need to design a system that allows us to easily plug in new permutations/variations
  • e.g. all of the fsharp permutations, targeting net20, targeting portableN, targeting vPrev FSharp.Core, with CoreCLR, with desktop CLR, with Mono, etc
  • This is extremely important: when we need to adopt a new target platform (e.g. CoreCLR or portable profiles), today it is very tedious and difficult to get test support. We want it to be a simple matter of creating some new "adapter" that plugs in to the test engine and rewires things appropriately.
• Still relies on command-line compiler
  • Should use hosted compiler where possible - faster/more flexible
• Tests which are really validating the runtime (e.g. tests\fsharp\core\lib) should be moved to the core unit tests

This is obviously a lot of stuff and won't be accomplished in one fell swoop. As with any bigger contribution, we recommend that it be planned out/discussed a bit ahead of time and worked on piece by piece in a fork.

[enricosada]

@latkin sry for late response, It's not a problem if this pr is not accepted (i understand your reasons), but let me address some of your points, maybe i can change your decision.

this pr:

• add a new test runner
• not remove the old one
• not change the tests files ( .fs or the tests permutation lists)
• is only a starting point

I understand there is a quality bar and you waste time on review/merge, but there is an improvement here ready to be applied, and it can open to new cleanup/improvements.
After merge, this can also run in parallel to old runner until ready (i'll fix issues you find and align if new tests are added).

I think is a good addition, not the whole solution to test suite, this is the the initial cleanup and a move forward. That's an area with really little love, i dont expect that to change soon.
I got some 👍 from @dsyme , @forki and @jack-pappas and the approach was discussed in the old codeplex forum.

i dont like long pr, easier merge a working base in master branch and iterate there ihmo, this pr only add code, so easy to revert.
If there are missing features, these can be added, put a list on an issue and let's discuss. a chat session too if possible (github issues are slow), but this pr is a good starting point ihmo.

• You want an hosted compiler instead of multiple run? fsc now is only a function ( do! fsc "%s -g --optimize- --target:library -o:lib.dll" cfg.fsc_flags ["lib.fs"] ) i can try to do that.
• You want multiple platforms? let me add an environment variable ( others like "debug/release" or fsc path are good additions)

all that can be added, but is easier after the merge, or i can continue this pr until done, np.

This pr doesn't change the tests (tests are also inside fsharp/fsharp), that's the premise of this work, i want to change the test runner.
There are 3 test runner (bat + perl + nunit), let's use only one (nunit is xplat and integrated in vs).

Also:

• a modern test runner (nunit)
• no other prerequisites (perl or bat)
• test are inside vs solution, easy to run or modify/cleanup (to unit test directly)
• easier to refactor because is easier to understand what a test do (permutations, actions)
• xplat ready, i need only to update the functions for fsc/fsi and some few stuffs.
• no modifications to tests (this is important, one step a time)
• run a test (or a permutation) easy. I know people can read test suite wiki, but is a friction (fsharpqa options are more obscure for non ms people).
• line by line translation of old .bat code, easy to check if correct

the others points:

• Still isolated/different from fsharpqa tests , yes because that was the easier test suite to move. to move fsharpqa and use same nunit runner (read env.lst or embed the file as fs function) is easy. I can do that after this one. convert perl script to fsharp, a new function and no more perl needed, easier to xplat too. we should refactor the new fsharp code, not the perl code.
• fsharp tests are a wild-west of ad hoc scripts and tools if test are in f#, is easy to see combinations used (like a dsl) and to cleanup one test a time. Also, if you see the code now like tests/fsharp/Commands.fs is not lot of custom tools
• Not portable/cross-platform compatible, is ready for xplat, i need only to change tests/fsharp/Commands.fs implementation but i can do that, and check tests
• Not flexible to different variations , to run on different platform, before run nunit, set an environment variable and read that programmatically to switch fsc/fsharp.core. Every configuration of nunit run, can be done like that. In development, inside vs, you can overwrite the values inside the global config script, or change a value in a tests.app.config
• Still relies on command-line compiler, the fsc is a function, call the executable or the hosted compiler is a choice.
• Tests which are really validating the runtime to unit test, sure, these can be moved after, but not this pr scope. but now is easy to understand what permutation of these test are run.

[enricosada]

@jack-pappas thx for help about that pr ( tweet, review etc ), really thx

@7sharp9 status is: waiting for @KevinRansom review.

The code is there (ported to NUnit 3), if we add that to master (yes master branch, not coreclr) it's only adding a new code, so it doesn't broke anything, it'is not enabled by default, can be run parallel to existing testsuite.

Later we can merge in coreclr.

I'd like to fix xplat ( fix exe name mostly ) but i need to have that merged before, is really a long effort (already rejected once). After that is fsharpqa suite (perl -> fs), easier that this one, because i can reuse the code.

[ctaggart]

👍 I like that plan. Getting that done would really help the xplat and coreclr efforts.

[dsyme]

Please add a comment about what this file is for.

[enricosada]

it's the 1-1 conversion from src/update.cmd.
Yuo are right, i'll add a comment with the original .bat file path in all converted .fs files, it's easier to understand.

[dsyme]

I think what I meant was "please take the time to record what this stuff is actually doing" :). I know the original cmd files weren't documented (and of course they should have been) - but now you're going through all this with such a fine toothcomb, it's a great opportunity to add a few basic comments recording what you've learned.

[enricosada]

@dsyme during the review pls take in consideration i added lots of helper functions, but that's because i tried to write .bat -> .fs conversion as simple as possible to review ( usually 1-1 ) .

[dsyme]

Why not use a for loop if processor supports processor.For?

[enricosada]

because my implementation doesn't work if i try to write as for loop.
Dont know why. That was something i wanted to ask for help
if you or someone else see the problem, can send me a pull request to this branch? i'll not rebase anymore this branch

[dsyme]

It's ok, we could fix it later.

[dsyme]

This is absolutely amazing work - I can see it's been a huge effort. It's a huge step forward and puts us well down the path of modernizing the "fsharp" suite to something much more palatable (and eventually cross-platform).

I personally would like to see all the *.bat files deleted as part of this change. (Somehow I don't think we should merge it if we don't delete the *.bat - either we are confident in this as the right direction - which I am - or not)

[dsyme]

@dsyme during the review pls take in consideration i added lots of helper functions, but that's because i tried to write .bat -> .fs conversion as simple as possible to review ( usually 1-1 ) .

Yes, you're right. You've done it exactly the right way. Some of the code comments are cosmetic and can be addressed later :)

[KevinRansom]

@enricosada

Hi, when I try to run the built tests using nunit I see this exception, any idea what I am doing wrong?

Unhandled Exception: System.Exception: Failed to load the test engine ---> System.InvalidCastException: Unable to cast object
of type 'NUnit.Engine.TestEngine' to type 'NUnit.Engine.ITestEngine'.
at NUnit.Engine.TestEngineActivator.CreateInstance(Version minVersion, Boolean privateCopy)
--- End of inner exception stack trace ---
at NUnit.Engine.TestEngineActivator.CreateInstance(Version minVersion, Boolean privateCopy)
at NUnit.ConsoleRunner.Program.Main(String[] args)

Kevin

[forki]

do we really want two different versions of NUnit? How about removing the old one?

[enricosada]

the old nunit is used in unit tests projects.
These proj are going to be converted to xunit instead of upgrade to nunit 3 in coreclr branch ( see #699 ) temporary or not.
Now nunit 3 final is release, so maybe is easier to upgrade the suite to nunit 3 final in all projects (usefull for coreclr too), but that's another pr

[enricosada]

@KevinRansom the nunit console and assembly is beta3?

[KevinRansom]

Okay ... I will pull this as it stands, can you take care of the nunit upgrade?

[KevinRansom]

Pulled to master and vs2015

[enricosada]

@KevinRansom GREAT!
i'll apply review feedback and upgrade nunit to final with other pr.

[panesofglass]

w00t!