Fix issue with tests, see #9617

[abelbraaksma]

Fixes #9617

This is an attempt to create better output on failing tests in ExprTests.fs, and this method may be useful for other tests in the CompilerService area:

• Instead of printing a big blob, now shows the exact line where a difference exists if test fails
• Re-enables and fixes two smoke-tests that seem to work now (may need disable for Mono)
• All temp files go in one location now, and are cleaned afterwards (function is in util location, easy to disable while debugging)
• This file used global state and module-level file-create inside global values. This is now delayed to allow proper cleanup and introspection
• Dump of referenced assemblies to help solve assembly-reference issues (like I suffered in Help wanted: how to run ExprTests in CompilerService, keep getting The exception has been reported. This internal exception should now be caught at an error recovery point on the stack. Original message: Exception of type 'FSharp.Compiler.ErrorLogger+UnresolvedPathReferenceNoRange' was thrown.) #9617), and any reference-related errors now print the actual missing reference.

Just hoping to make it easier to see the woods for the trees with these tests, as they've blocked my progress a bunch of times and possibly others.

I realize that some of these tests may be removed, as @cartermp pointed out in #9617, but while that isn't the case yet, I hope this will make life a little easier ;).

Related PR: fsharp/fsharp-compiler-docs#766

Before, any differences were utterly senseless in the output, because it didn't point to the actual difference. After, it shows up like this (and I think there'll be other places that can benefit of this change):

And they now run handily and correctly in VS IDE Test Runner:

Referenced assemblies are now dumped, which can help locate issues when those in-process compilations fail due to missing references:

[smoothdeveloper]

Checking those differences in test results, and adjusting the expected values in code + recompile is a reason I generally prefer test using baseline files, easier to diff and faster to run test after adjustments.

Even if the nunit string comparison now shows better, it still only shows the first difference and only few characters, merging the change to fix the test remains a lot of trial and errors.

[abelbraaksma]

is a reason I generally prefer test using baseline files, easier to diff and faster to run test after adjustments.

@smoothdeveloper I totally agree, but I didn't want to introduce a complete new testing method. Besides, testing is being refactored largely atm, and your idea of baseline files sounds a good way forward (while still maintaining the point-to-exact-error-location, preferably). @vzarytovskii, what do you think?

it still only shows the first difference and only few characters, merging the change to fix the test remains a lot of trial and errors.

Correct. But before this I was utterly in the dark and had no idea what to fix. Since all these compare-strings are inline in the source file, it is next to impossible to fix such tests (it literally blocks me from moving forward for this: #9549).

I was looking for a quick way to make this at least a little more manageable. Considering that these tests may get a complete overhaul and/or are dropped, I didn't want to spend too much time with them.

Bottom line: certainly not a perfect solution, but it does give a little bit more grip for contributors (I hope).

[KevinRansom]

@abelbraaksma

Failing test:

https://dev.azure.com/dnceng/public/_build/results?buildId=716583&view=logs&j=c3b5cd2e-ddad-540a-7ae1-df25e52fdba0&t=d4985d65-7d01-5e76-2879-f1b5d6535baa&l=13051

[abelbraaksma]

@KevinRansom, tx. I committed a possible fix.

While these are now easy to fix, I really wish there was a way to run a test locally "like in the CI". This happens quite a lot that things are different, and it would be nice to catch them before submitting.

And there's the consideration that this reflection-stuff apparently behaves slightly different between runtimes, which is a bit worrisome.

[abelbraaksma]

All things considering, and in particular @smoothdeveloper's comments, one alternative way is to have these tests work on a TestSource, which should then be these "expected strings" that are being tested against. The advantage being that then there would be one test per line. If 10 lines fail, 10 tests fail and it is easier to fix.

The current way of having literally over a hundred subtests in a single test is not very maintainable.

[cartermp]

Tests like this are horrible to maintain, in part because they're not testing a single thing, but many things. for this specific kind of test, comparing output against some test data (we can call them baselines) could be a little better, but the issue isn't that but the fact that these mondo-tests are asserting so many things.

[abelbraaksma]

@cartermp, I couldn't agree more. And if only the generated strings (from reflection?) were the same on every system, then at least we'd have "fixed locally & fixed CI". But since it isn't (see my last three commits), it's a lot of "fixed locally, failed CI".

An exercise in patience :).

Still, these tests do check for accidental changes we wouldn't want, so I can understand why they're here.

[abelbraaksma]

@KevinRansom, @cartermp, yippee! Finally green :)))))))

Ready for review 🙇

[cartermp]

Thanks, this is a good improvement that should help some recent PRs where we improve optimizations.

[KevinRansom]

Cleaning up is a good thing, but the name reuse is a problem. What I often do Is a bit belt and braces: https://github.com/dotnet/fsharp/blob/master/src/fsharp/FSharp.DependencyManager.Nuget/FSharp.DependencyManager.fs#L128 ```` let key = "nuget" let name = "MsBuild Nuget DependencyManager" let scriptsPath = let path = Path.Combine(Path.GetTempPath(), key, Process.GetCurrentProcess().Id.ToString() + "--"+ Guid.NewGuid().ToString()) match outputDir with | None -> path | Some v -> Path.Combine(path, v) let generatedScripts = new ConcurrentDictionary<string,string>() let deleteScripts () = try #if !Debug if Directory.Exists(scriptsPath) then Directory.Delete(scriptsPath, true) #else () #endif with | _ -> () let deleteAtExit = try if not (File.Exists(scriptsPath)) then Directory.CreateDirectory(scriptsPath) |> ignore true with | _ -> false ````

[KevinRansom]

System.Numerics reference.

[abelbraaksma]

System.Numerics reference.

Done, it is in a more sensible place now, as you suggested.

What I often do Is a bit belt and braces:

I chose something similar, with threadid and procid, it's still fairly simple.

[abelbraaksma]

@vzarytovskii, since you're working with the tests, this may be of interest to you too. If these tests aren't removed but stay in place, the approach here may help make it less painful to work with these by showing more precise errors in the log, and the actual line that went wrong.

Ideally, however, as pointed out by @smoothdeveloper, the textual in-file contents of these tests is troublesome and placing them in some *.bsl style files, or with TestSourceAttribute would probably go a long way to improve the experience with this set.

Either way, these changes have now only been applied to ExprTests.fs, but are applicable to the rest in this directory. Feel free to ping me once you get to these tests to see how we can further improve on them.

[vzarytovskii]

@vzarytovskii, since you're working with the tests, this may be of interest to you too. If these tests aren't removed but stay in place, the approach here may help make it less painful to work with these by showing more precise errors in the log, and the actual line that went wrong.

Ideally, however, as pointed out by @smoothdeveloper, the textual in-file contents of these tests is troublesome and placing them in some *.bsl style files, or with TestSourceAttribute would probably go a long way to improve the experience with this set.

Either way, these changes have now only been applied to ExprTests.fs, but are applicable to the rest in this directory. Feel free to ping me once you get to these tests to see how we can further improve on them.

Thanks for pinging me, right now I'm working on a simplified, easier to use and more composable compiler api for tests.

So far I've touched only simpler apis, such as compilation, type checking, baseline testing.

I will ping you once I will get to those tests and we can discuss the improvements in details.

[abelbraaksma]

So far I've touched only simpler apis, such as compilation, type checking, baseline testing.

Inasmuch you can call that simple 😆

I will ping you once I will get to those tests and we can discuss the improvements in details.

Gotcha 👍

[abelbraaksma]

Some tests count the number of references, and are dependent on the default set included in Common.fs:

2020-07-11T16:20:48.8627471Z   X Test ManyProjectsStressTest whole project errors [2s 743ms]
2020-07-11T16:20:48.8629146Z   Error Message:
2020-07-11T16:20:48.8630983Z      Expected: 104
2020-07-11T16:20:48.8631784Z Actual: 105
2020-07-11T16:20:48.8632187Z   Expected: 104
2020-07-11T16:20:48.8632837Z   But was:  105

and

2020-07-11T16:20:50.3758640Z   X Test multi project 1 whole project errors [5ms]
2020-07-11T16:20:50.3760196Z   Error Message:
2020-07-11T16:20:50.3761321Z      Expected: 6
2020-07-11T16:20:50.3761601Z Actual: 7
2020-07-11T16:20:50.3761956Z   Expected: 6
2020-07-11T16:20:50.3762167Z   But was:  7

Another gotcha: these tests are not in the VisualFSharp.sln, but can be found in projects with almost identical structure in FSharp.Compiler.Service.sln. There may be a reason the file MuiltProjectAnalysisTests.fs is not included in the main application, so I'm hesitant to make that change.

Anyway, new commit should fix these "hidden" tests.

There are more "hidden" tests that don't appear in the main solution:

Let's hope the latest commit makes things green again.

[abelbraaksma]

All green, @KevinRansom, this is ready for rereview.

[abelbraaksma]

ping @KevinRansom ;)

[cartermp]

@vzarytovskii can you take a look at this while @KevinRansom is out on vacation?

[vzarytovskii]

Changes look good to me!

[abelbraaksma]

@vzarytovskii, thanks. Can you, or @cartermp, clear the status of the requested changes by @KevinRansom (all done), so that this can be merged while he's on holiday?

Resolved