[WIP] Migrate "fsharpqa" suite to use NUnit

[enricosada]

Use nunit instead of perl to run tests\fsharpqa suite ( ref #697 )

some tests are missing, other fails, but the tests\fsharpqa\Source\run.pl script is pretty much converted from perl to f#

fsharpqa tests is only a single script, so fixing the runpl method, should make it pass all

Some notes:

• nunit 3
• same configuration of cambridge suite ( path executables like fsc, fsi etc ), it's the same code 😄
• can be run in parallel with old perl runner, so easier to check migration
• test runs in parallel by directory ( like cambridge )
• test can be run/debugged inside visual studio test runner

i'll implement only what's used now by fsharpqa test.
For example syntax like //# Expects: success : text it's not used => not implemented.
Only xml like <Expect status="success" ></Expect> syntax is supported

When possible i'll fix test spec instead of use a more forgiving parser. That's because code is more clean, and it's easier to spot bug in the parser/script
An example is the attribute status, was case insensitive, now is required lowercase.
I'll change tests like that with FIX TEST SPEC commit message ( like a86bb3b or a86bb3b ).

comment/code with "not implemented" mean it's going to be fixed
"not supported" mean i checked fsharpqa sources and it's unused => not going to implement it

Hosted compiler will be the last, because i think run in parallel by directory can be fast enough, so maybe it's not needed

@KevinRansom @otawfik-ms the internal bat script set some special env var before call runall? for example X_SKIPFULLDIAGCHECK or others like that?
some stuff are useless, like VISTA_ONLY
some dont know, like SIMULATOR_PIPE, it' a test helper?

[dsyme]

I've been chatting to @enricosada.

The aim of this PR is to move the "fsharpqa" quite off using Perl at all and over to using NUnit. I think that will be an absolutely fantastic result and a good step towards simplifying our tests: it will finally become clear that we have one test runner framework (NUnit) and we will move from three test runner languages (F#+Nunit, Perl, BAT) down to one (F#+NUnit).

So I'm very supportive of the direction. The day we can remove the vast majority of *.bat and all *.bat files will be a great day, and massively helps with x-plat testing too.

[isaacabraham]

+1. I struggled to get the tests running with that perl stuff - sticking to NUnit and F# massively lowers the barrier to entry to getting more people involved.

[enricosada]

i think perl/bat it's not the real problem (bat is a problem for xplat ok, fshapqa it's better). Runtest.cmd works, it's console based, but ok ( i use it a lot )

The problem it's the custom dsl built to configure fsharp and fsharpqa test, that require to learn the syntax and rules, that's not needed. for fsharp suite there is also permutations of fsi/fsc flags, and custom rules about skip/execution in the single-run.bat files.

This pr remove the need for perl, and custom runner (we can use vs test explorer or nunit gui or nunit console)
The fsharp and fsharpqa dsl continue to be used in fsharpqa, but now we can easy migrate every single test to be dumb, without issues (each test print to test output the command who run (fsc source --bla), so it's very easy

An example of fsharpqa dsl:

ReqENU  SOURCE=gccerrors01.fs COMPILE_ONLY=1 SCFLAGS="--gccerrors --nologo gccerrors01.fs >gccerrors01.txt" POSTCMD="\$FSI_PIPE --nologo --quiet --exec comparer.fsx gccerrors01.txt gccerrors01.bsl"   

and

<Expects status="error" id="FS0517" span="(9,1-9,3)>End of file in string embedded in comment begun at or before here</Expects>

that's ok, but with a f# test

• run fsc gccerrors01.fs --gccerrors --nologo
• fsc should exit with 1
• comparer output of compiler

it's easier to understand. and standalone, no need for configurable script

my idea of test is something like tests/fsharp/fsc/warnings/FS2003/Warning_FS2003.fs. Can be improved, but it's easy, run fsc with some code, check output and results.
An nice thing i think it's the use of an attempt (processor) computation expression, like in let! result = fscToLibrary "%s --nologo", to do easy check (a cmd with exit code 0)

let! result = fscToLibrary "%s --nologo" cfg.fsc_flags { 
            SourceFiles = [ SourceFile.Content("test.fs", code) ]
            OutLibrary = "lib.dll" }

also there is a different dsl for tagging/skipping tests (that's a simple [Category("ReqENU")] in nunit)
Tagging/skip can be also done with nunit attributes, instead of env.lst files, one test a time, removing from env.lst and adding as Category.

That's phase 2 of cleanup, for now at least we can run all tests xplat with nunit 😄

[isaacabraham]

@enricosada think the main point I was making was more - I'm an F# developer. I want to work on the compiler. I want to use F# to do as much of this as possible :-)

[enricosada]

yes @isaacabraham me too 😄

[dsyme]

I'm confident this is a good direction.

The DSL is not too bad - it could be worse. I can definitely see how the test engineers who created it landed there because it's the kind of thing that would have been needed in previous C# and C++ projects. But neither Perl nor ihe DSL are necessary when F# is available as a test engineering language, and it definitely raises the bar for adding tests - yet another thing you need to know.

The .bat files in "tests\fsharp" are, on the other hand, un unspeakable monstrosity. I'm the fool responsible for the first use of a .bat file to automate testing for F#. Way back in 2003 or something. I can't believe I did that, and I'm deeply sorry for all the pain it has caused ever since (including regressions missed because of failing tests not being logged properly). I'll be deliriously happy when they are removed!

[dsyme]

@enricosada what's the status on this? Thanks

[dsyme]

Just wondering if it's still WIP as marked, or proposed to merge. We still need to review.

[dsyme]

Is this a real TODO?

[enricosada]

i need to check usage when i see a test using it.
I cannot findSKIPTEST in tests, so the first part it's unused. The run.pl support it, but it's not used by current fsharpqa tests, so useless for us, no need to implement it.
The else it's used to skip tests by platform, but maybe it's useless (skip test because os < vista it's useless now for example).

I added a todo to review usage.
i'll change to make the test fails if the condition it's true, so it will fail the a specific message and we can review it

[enricosada]

it's a real WIP, but good enough for an early review.

The only big missing feature are tests with output redirection, i need to parse command like > out.txt &2>1 to convert as f# RedirectTo.OutputAndError(Overwrite("out.txt")). i'll do it soon

@otawfik-ms sent a ping to me (thx mate) about the issue of fsharpqa migration, because i was doing it in background without a public branch, so we are not duplicating work

I need some feeback about pr description #872 (comment) /cc @KevinRansom

[enricosada]

@dsyme About fsharpqa dsl, i dont think it's bad (maybe i was too negative in the feedback), it was a really good tradeoff about configurability and scripting. the runner it's ok too (run test with tag, skip by platform, etc).
But with nunit now as test runner, and f# as test language, it's obsolete.

[enricosada]

@KevinRansom it's already merged? 736e9c1

[forki]

what's the status here?

[enricosada]

70%. I think this week or next is done, i am working on it and that's the third time i say in standup "next week is done" but i am really near.
I need to have the right mood for this one, it's a bit boring 😄

[dsyme]

Getting closer! :)

[enricosada]

current status: 80 failing tests of 2900, plus 120 skipped because i didnt implement MODE=FSI_PIPE yet

i am checking each failure, it shouldnt take too long, usually it's a stupid thing

when i am near 40 (and implemented FSI_PIPE) i'll add the suite to runtest,cmd so we can use jenkins/appveyor

btw @dsyme the Conformance/TypeForwarding/Struct (and others) are missing recomp program, what's that? i didnt find it. it's used as POSTCMD running CheckRuntimeException.bat

is32bitruntime.exe
IF ERRORLEVEL 1 (recomp /p:%1 /ee:%3) ELSE (recomp /p:%1 /ee:%2)

[dsyme]

btw @dsyme the Conformance/TypeForwarding/Struct (and others) are missing recomp program

I didn't write these tests, I'm not sure what this is meant to do.

[KevinRansom]

@enricosada is this dead?

[enricosada]

@KevinRansom no, is not dead, just a bit sleepy 😄 thx for ping

i think next week after the mvpsummit i'll start back on working on the tests pr (this one, fsharp suite and remove bat), because i got amazing help from community about .net core, so i have lots more of free time.

just leave this open open for a bit more

[dsyme]

I'll close this for now. Taking to @KevinRansom and @enricosada we think the best way to go is to add a generic test runner to the "fsharp" suite that understands the env.lst and perhaps test.lst files of rht "fsharpqa" suite, and move all the tests over to the "fsharp" suite that way. Then delete a the perl :)

[dsyme]

@enricosada I expect large chunks of this will be reusable under that approach?

[enricosada]

Yes. Less nunit attribuite Logic, just calling the translated perl script as a function passing env.lst content (copied inline , not reading the file).
I have done f# for next dotnet sdk preview, going to work on that this weekend

[smoothdeveloper]

@enricosada / @dsyme can we somehow rediscuss some of the details of what we (/do not) want in future?

for me:

want

• leave all env.lst in place (reused by having the parser @enricosada made) (this makes the PR small)
• a single [<Test>] ``run all fsharpqa tests`` for CI
• a single [<Test>] ``try me single test`` for testing just one of the env.lst file, possibly ability to run that right from FSI is a plus as well (the person making contrib to fsharpqa just replace the name of env.lst folder to run
• do the best we can with current env.lst DSL to get error message that help fixing the test

don't want

• I don't want to write any F# to add a test based on env.lst established framework
• I don't want to maintain hundreds of F# tests for refactoring down the road
• something which is not 100% declarative, we can add "one-off" test when the DSL doesn't fit as well, but having hundreds of (even small) NUnit tests looking alike, I feel, is not the best solution / practice

When this is in place, and both CI does same as perl, and contributor pain is reduced due to removing perl dependency / ability to run single or set of tests easily, then we can start looking at the knowledge embedded in the DSL and make it more accessible or design a v2 for future suite of declarative tests

[enricosada]

@smoothdeveloper we spoke about this on slack, but i was on mobile, so probably i missed to add some info.

Large part of this pr is to normalize the xml inside fsx, to make parser work, plus edge cases.

The env.lst is used already now in this PR, it's the base of fsharpqa runner, env.lst (test specification) + xml inside .fsx files (test asserts and expected result).

I dont want to change that. if you add another line to any env.lst, another test will be added to suite (if similar), no f# code changes needed.

Lot of code can be shared, because the runpl reimplemented from perl as f# , is shared with all tests (unlike tests/fsharp suite, where lot of bat where custom).
There is also a mechanism to override commands, so edge case works xplat (bonus) and more important without invoking arcane bat commands to do stupid thing (if implemented in f# 😄 )

I splitted in one [<Test>] per directory, because i was implementing that, so i was not sure if a test was ok or needed special care. And my FSharpQATestAttribute worked with just one directory because was easier to implement 😄 No hard reasons. so lots of tests can be collapsed as one.

I like having more tests (not one each env.lst line, but one each directory), because make it easier to refactor it as needed.

Speaking with @dsyme some time ago the feedback (already applied by dsyme and kevin to test/fsharp suite after my initial conversion pr, similar to this one):

• make it fsi friendly
• remove nunit magic (the attributes)
• raise exception and stop instead of attempt
• reuse work done in test/fsharp suite, like config (who is already linked as source file)

pratically make runplWithCmdsOverride not implicit, but explicit.

from nunit magic attribute:

    [<Test; FSharpQASuiteTest("CodeGen/StringEncoding")>]
    let StringEncoding () = 
           runpl |> check

to:

    [<Test>]
    let StringEncoding () =
          let { Directory = dir; Config = cfg } = testContext "CodeGen/StringEncoding" //same of test/fsharp suite
          let vars = envLstData dir //return list of test to run, pratically env.lst rows
          runpl dir cfg vars

Lot can be improved later. But to add a new test directory, is just this code.

When dsyme say embeed env.lst, mean

    [<Test>]
    let StringEncoding () =
          let { Directory = dir; Config = cfg } = testContext "CodeGen/StringEncoding" //same of test/fsharp suite
          let envLstDataLines = 
                [  """ SOURCE="dummy.fs testcase.fs" PRECMD="\$FSI_PIPE --exec NormalizationFormKC.fsx > oracle.cs && csc oracle.cs && oracle.exe>testcase.fs"	# FormKC """
	           """ SOURCE="dummy.fs testcase.fs" PRECMD="\$FSI_PIPE --exec NormalizationFormKD.fsx > oracle.cs && csc oracle.cs && oracle.exe>testcase.fs"	# FormKD """
	           """ SOURCE="dummy.fs testcase.fs" PRECMD="\$FSI_PIPE --exec NormalizationFormC.fsx > oracle.cs && csc oracle.cs && oracle.exe>testcase.fs"	# FormC """
	           """ SOURCE="dummy.fs testcase.fs" PRECMD="\$FSI_PIPE --exec NormalizationFormD.fsx > oracle.cs && csc oracle.cs && oracle.exe>testcase.fs"	# FormD """ ]
 
          envLstDataLines
          |> List.iter (runpl dir cfg)

because as you can see, the envLstDataLines are just a raw method to initialize the f# type, used by runpl function.

    [<Test>]
    let StringEncoding () =
          let { Directory = dir; Config = cfg } = testContext "CodeGen/StringEncoding" //same of test/fsharp suite
          let envLstDataLines = 
                [  { SOURCE = ["dummy.fs"; "testcase.fs"]; 
                     PRECMD = "\$FSI_PIPE --exec NormalizationFormKC.fsx > oracle.cs && csc oracle.cs && oracle.exe>testcase.fs"; 
                     TAGS = ["FormKC"] }
                   ... ]
 
          envLstDataLines
          |> List.map parseEnvLstDataLine //same function to covert string -> EnvListLine type
          |> List.iter (runpl dir cfg)

Like that, the env.lst is not magic anymore, because intellisense works too 😄 And env.lst file can be removed
For example the initial env.lst whitespace, because parse reasons...

Stop read here if you want, the following is just a suggetion for future work and possibilities to cleanup ihmo

The following is not a target now, but @dsyme and @KevinRansom can choose to do it later, if they want. i think is good for maintanability, because you dont need to learn runpl behaviour (copy paste is good, but read f# code is easier)

Fist step is remove envList magic, to remove the need of runpl algorithm (who undocumented, and with some implicit default behaviour).

the Libraries/Core/Collections test for example:

	SOURCE=list_head_tail01.fs SCFLAGS="--test:ErrorRanges"			# list_head_tail01.fs

can become (like in test/fsharp suite). Note it doesnt call runpl anymore

    [<Test>]
    let Libraries_Core_Collections () =
          let { Directory = dir; Config = cfg } = testContext "Libraries/Core/Collections"

          let result = fsc dir "--test:ErrorRanges" ["list_head_tail01.fs"]

          let asserts = getAssertsFrom "list_head_tail01.fs" //parse xml and read asserts list

          asserts
          |> List.iter result

Or if we want to remove xml specification too:

//<Expects status="error" span="(30,6-30,8)" id="FS0039">The value, constructor, namespace or type 'hd' is not defined</Expects>
//<Expects status="error" span="(31,6-31,8)" id="FS0039">The value, constructor, namespace or type 'tl' is not defined</Expects>

We get:

    [<Test>]
    let Libraries_Core_Collections () =
          let { Directory = dir; Config = cfg } = testContext "Libraries/Core/Collections" //same as test/fsharp suite

          let result = fsc dir "--test:ErrorRanges" ["list_head_tail01.fs"] //same fsc function of test/fsharp suite

          result.ExitCode |> Assert.Equals 1
          result.Errors |> CollectionAssert.Exists { ErrorId = "FS0039"; Span = ( (30,6), (30,8) ); Text = "The value, constructor, namespace or type 'hd' is not defined" }
          result.Errors |> CollectionAssert.Exists { ErrorId = "FS0039"; Span = ( (31,6), (31,8) ); Text = "The value, constructor, namespace or type 'tl' is not defined" }

The rest of test is the same. i just removed the need to understand custom xml assert sintax, and custom env.lst and runpl logic. simple commands and assert. With intellisense.
And there is no more two different test suite, but only one.

So 👍 for f# dsl, 👎 for xml/custom format (undocumented and used only here) dsl

As a note, this is becoming more similar to expecto style than nunit attribute based, and we can leverage some features of expecto in future maybe.

[smoothdeveloper]

@smoothdeveloper we spoke about this on slack, but i was on mobile, so probably i missed to add some info.

Ouch, sorry.

I dont want to change that. if you add another line to any env.lst, another test will be added to suite (if similar), no f# code changes needed.

So, there is codegen taking env.lst and generating vanilla F# test suite (but generated code won't be committed in repository)? If that's the case that's a novel approach I never considered and would work quite well also :)

So trying to understand, with your work, do we have to write on top of creating MyNewTest folder with env.lst:

    [<Test>]
    let MyNewTest () =
          let { Directory = dir; Config = cfg } = testContext "path/to/MyNewTest" //same of test/fsharp suite
          let vars = envLstData dir //return list of test to run, pratically env.lst rows
          runpl dir cfg vars

or this is actually re/generated automatically based on the folder structure?

[enricosada]

So, there is codegen taking env.lst and generating vanilla F# test suite (but generated code won't be committed in repository)? If that's the case that's a novel approach I never considered and would work quite well also :)

No codegen, i just parse the lines and execute functions based on line text.
env.lst string -> EnvLstLine.Data type (type safe version) -> passed runpl to use it (so $FSC_PIPE program.fs -> run fsc with source program.fs ).

So trying to understand, with your work, do we have to write on top of creating MyNewTest folder with env.lst or this is actually re/generated automatically based on the folder structure?

Not automatically generated (i hate codegen for tests, and suite should be dumb if possibile ihmo).
All test code needed for whole fshapqa is already defined in this pr.

Because you maybe need to add overrides to some commands. It's easier for translate old tests (to be sure are not different :D)

For example BuildAssembly.bat (no xplat, bat file) can be overridden with

    | StartsWith "BuildAssembly.bat " code ->
        Some (``Conformance-TypeForwarding-Cycle-BuildAssembly``.run code)

and that Conformance-TypeForwarding-Cycle-BuildAssembly can be an 1 on 1 line translation from bat to f#, ref BuildAssembly.bat.fs

So will run xplat, and maybe easier to read, because bat files are not bash script, are complicated/arcane to do text processing

[smoothdeveloper]

Thanks for the precisions, I agree code gen is not great, but adding hundreds of small test code is not great either IME (a big pain to do changes of infrastructure due to coupling in many small snippets of test code).

I prefer to keep a declarative framework (env.lst for now), have better tooling around it, and a single test (that we don't change) which loops all and give a nice print out of errors.

For "one-case" kind of tests, we can leverage the same infrastructure (with additional params) but the default story for developer should be:

• edit env.lst
• add folder
• push PR with new test

not touching any F# code or recompiling, and for convenience we can have a .fsx to run test given a single env.lst where we just hardcode the folder name (can use FSharp.Management.FileSystem TP even).

Is what I'm describing a goal we would consider?

[enricosada]

@smoothdeveloper sure, having a test with "env.list line string" -> run is ok to have.

But we are using nunit atm, big code for in a single tests is noisy for runner and result.
Atm test seems only one for an env.lst but i was duplicating tests programmatically, using nunit data test support.
If we use expecto, will be perfect, works exactly as you want