Add F#+ to the test suite

[realvictorprm]

As discussed in #3814 (comment) this PR adds the build of F#+ to the test suit.

Left points are:

• Reference correct commit of F#+
• Run F#+ tests too

The changes include:

• The Git exe is retrieved from the Path environment variable
• A new directory ./tests/fsharp/repos/ is created.
• F#+ is cloned to this directory and reset to a specific commit
• Build of F#+.fsproj

Signed-off-by: realvictorprm mueller.vpr@gmail.com

[realvictorprm]

As soon as the CI passes this is ready.

[realvictorprm]

This is ready.

cc @dsyme

[forki]

We are now verifying downstream project builds as part of compiler builds?
Why not copy the source code files of the test suite?

[realvictorprm]

The whole project itself is a great stress test for SRTP's but we don't want a static test in here. If we change SRTP behaviour F#+ should adapt too and continue acting as stress test. At least this is my understanding and opinion.

[dsyme]

I'm hugely in favour of adding builds on known stable commits of existing F# projects.

I'd love to see the same thing added for FSHarp.Data and several other projects.

Adding a build of FSharpPlus was a specific request of mine to help pin down SRTP behaviour

[baronfel]

Kevin had something promising for finding the dotnet exe in #5882 I believe

[realvictorprm]

I'll have a look whether I can use it :)!

[realvictorprm]

For now I'm searching the dotnet exe using the Path environment variable. This should be more x-plattform.

[dsyme]

@realvictorprm What's the failing test here, do you know what the issue is?

[realvictorprm]

It looks like that the output directory for the FSharpPlus.Tests.dll is on the CI different to my machine.
I've got no idea where it's located. As result the nunit command runner can't find the dll and so fails to run the FSharpPlus.Tests

[realvictorprm]

I think I fixed what was wrong now. Let's see whether the CI passes now.

[realvictorprm]

I think F#+ is a project with special placement in the F# community. I don't see why enforcing it to be working always is wrong. So what fits best in my opinion is option 3.

[KevinRansom]

@dsyme only having yet another suite of tests that none of us truly know.

[cartermp]

@realvictorprm (and @dsyme) My pushback is not against this ensuring we don't break FSharpPlus. It's that I do not wish us to be in a questionable position due to FSharpPlus. Introducing a variable we cannot control into our own CI system does not feel appropriate at all.

I'd be for building an "OSS integration suite" that need not necessarily pass to allow us to still be green, but I don't see that as a particular priority.

[gusty]

@cartermp FSharpPlus is not a variable you can't control.

You can stick to a specific version of it (making it a const instead of a variable), and there shouldn't be any reason to break it, until we decide on breaking changes on the compiler, in which case it would help us to transition in a controlled way and have a good idea of the impact and the trade-off of those changes.

Also, F#+ lives in fsprojects so anybody in the org can eventually take over if a bus hits me and the other maintainer.

[dsyme]

@cartermp Yes the testing is against a specific commit (I'll re-review it to make sure).

AFAICS there are literally no variables here (except the continued existence of github and the availability of an Internet connection, both of which can be assumed) . What's there in that commit should simply continue to work. If it stops working we should take a responsible action (disable the tests, update the commit, or reject the PR that breaks it).

I think the critical thing is that compiling and running the FSharpPlus tests would (for me) be a requirement for any future change or bug fix in SRTP. Given this, we may as well automate it. It is simply critical to know when we are breaking existing code early rather than late. TBH if we'd been doing this for a few other repos too we would have saved ourselves months of handling regressions. And it's not like what is being done here is complex, it's just a git fetch and build...

Let's embrace this. It's a crucial step towards bug fixing for SRTP and advancing any future type-class like work of any kind.

[dsyme]

but I don't see that as a particular priority.

Specifically getting FSharpPlus compiling and tests running in CI is documented as a priority for me in how we advance SRTP bug fixing. I think those priorities are right for that particular area given past history and where we're at.

I'm not keen on us shipping regressions and feel we need to test against code that is known to stress features. We would have avoided several regressions this way. The coverage offered by FSharpPlus is seriously valuable to efforts to advance the language design in the hardest of areas.

From my perspective, a snapshot of the FSharpPlus tests would also be ok. However it seems simpler just to have a general ability to grab known commits of real-world projects.

[dsyme]

You can use the --branch argument to git clone to clone at a specific commit

[cartermp]

@dsyme My worries are:

• Testing against a specific commit will be fine until a relatively short amount of time in which we get an issue files on us to do a different commit. F#+ isn't frozen in time and it's only a matter of time before we're asked to test against a future commit
• Our requirements around shipping will eventually mean that we must commit and ship something ASAP, and if we're entirely self-consistent but there's some issue with running F#+ tests (environmental, something wrong in their tests, etc.) then we're knowingly doing so while red. That defeats the entire purpose of CI
• We do not know the tests of this source code at all, and eventually we'll make a change that requires changes in those tests. What do we do if someone cannot help adjust them? Is it then our responsibility to learn the F#+ source code and tests? I think the answer is absolutely not
• This further increases the extremely lengthy CI we already have, which is already a problem for us. 40+ minutes just to see if we can merge in a simple fix is not okay, and tacking on a download -> build -> run of tests from an uncontrolled source does not make that problem better
• Is it even allowable for us to do this, given that the CI servers are Microsoft's resources? I'm not even sure...

I think that if we want SRTPs to be better-tested, we should include more tests in our own source. I don't see the connection this source code has with any typeclass-like work either, as this library is not the source by which typeclasses would arrive within the language.

Specifically getting FSharpPlus compiling and tests running in CI is documented as a priority for me in how we advance SRTP bug fixing.

On a mirror of VisualFSharp, this wouldn't be controversial. But we should have been consulted on this earlier and no plan should have been put together before we're in agreement about the scope and ramifications of this.

I'm for using OSS libraries as a smoke test. We do that with SAFE when we assemble a build of things on .NET Core that is ready for validation before a release. But this is a very different thing compared with running third-party code in our own CI.

[gusty]

Some comments from my viewpoint.

Testing against a specific commit will be fine until a relatively short amount of time in which we get an issue files on us to do a different commit. F#+ isn't frozen in time and it's only a matter of time before we're asked to test against a future commit

Let's don't get it the other way, if we decide to update to a different commit of F#+ in the future, it will be driven by the interest of the compiler, not the other way. The goal is to test the compiler, not F#+.

... running F#+ tests (environmental, something wrong in their tests, etc.) then we're knowingly doing so while red. That defeats the entire purpose of CI

There's nothing environmental in those tests, F#+ has only referential transparent functions, and that's what is being tested.

and eventually we'll make a change that requires changes in those tests.

Well, in that case, it would be a clear decision of making breaking changes.

This further increases the extremely lengthy CI we already have

Yes, but the CI will still be too long with or without this.

[dsyme]

@cartermp@KevinRansom

It's ultimately Microsoft's decision about whether it accepts these tests into this repo. I've given my reasoning as language designer ("no fixes or change to SRTP until we are doing automated testing of the FSharpPlus tests").

To be honest I don't care if these are smoke tests or CI tests - we just need to know the status before we make any changes. That's all I personally really care about as language designer. The engineering is up to you guys.

I'd appreciate it if you work with @realvictorprm on how/if this can be advanced.

...But we should have been consulted on this earlier ...

Well, let's take this discussion as that consultation. To be honest no one expected this to be controversial and it is still a little eye-opening that it is - but I respect your reasons and you obviously stand by them even on deliberation.

I don't see the connection this source code has with any typeclass-like work either, as this library is not the source by which typeclasses would arrive within the language.

There are two angles to this

• any future work on typeclasses must align with the SRTP mechanism to some extent. That is, TC-like constraints must be able to be used in conjunction with SRTP constraints in sensible ways. TC constraints may also allow SRTP constraints to be implied. I would thus be very reluctant to do any work on TC in F# until the SRTP testing is improved.

• the approved path forward for SRTP is to allow type extensions to satisfy SRTP constraints, see Allow Type Extensions and Extension Methods to Satisfy Constraints fsharp/fslang-suggestions#29 and [WIP, FS-1043] Extension members visible to trait constraints #3582. My preference is not to admit that work until the plan listed in my comment here is enacted, the first step of which is to improve the SRTP testing.

Anyway, in both cases my recommendation is to improve the SRTP testing in the repo. From where I sit, by far the easiest route to improve SRTP testing seems to be to run the FSharpPlus tests in CI. But that's just my opinion.

[KevinRansom]

@dsyme, we will add them, they add a lot of value, but please allow me the leeway to whinge about more test frameworks, I am a Pom after all. Kevin

[cartermp]

I still feel quite strongly that if we want a better regression suite, we should write a better regression suite. If we're not certain of our own quality, we need to address that in our own tests. If we're never going to change the specific commit that we want F#+ to be tested against, we should just wholesale copy all of what it does into our own test suite. That would save additional time in our CI run anyways.

What we could do, if we're interested in a general smoke test for various blessed OSS libraries, is set up a separate CI run that does not have a say on if a PR is green or not. That could take the built compiler and build certain projects, reporting back success/failure. I'd be in favor of having that point to latest master, since this could also be seen as a bit of a partnership between us and the library authors. It has worked very, very well with SAFE and I think it could work well here, too.

[KevinRansom]

@cartermp We are open source. This is what writing our own better regression suite looks like. Kevin

[cartermp]

No, it quite literally does not.

[realvictorprm]

OK Ok fellows. Fine enough let's have another CI build reserved for building OSS libs.

PS:
I don't want to make a big deal out of adding F#+ as test. I want to continue working on SRTP fixes as fast as possible as there is enough left to do.

[KevinRansom]

@realvictorprm it's not a big deal.

[realvictorprm]

👍 Feel free to ping me on slack for details on the CI. I'll be working tomorrow most likely 8 hours on the compiler 😃

[KevinRansom]

I can promise you I am unlikely to get this tomorrow.

[realvictorprm]

@KevinRansom would Friday be better? I can shift my work hours one day.

[dsyme]

No, it quite literally does not.

I can appreciate this perspective. I personally like to combine both: lots of user code, great test suites.

@cartermp How would you feel about a copy-and-paste of existing FSharpPlus into our test suite, where I personally went through and assessed each test for "is this a good regression test for visualfsharp"?

[cartermp]

@dsyme I would definitely feel comfortable with that, so long as you're confident that the snapshot at which that is done is sufficient for catching regressions.

[KevinRansom]

@dsyme I think this it is fine as is. We can tweak it if it becomes a problem.

Compared to fcs this will be cake.

[KevinRansom]

@realvictorprm , don't worry. Groveling in other code bases is something I am quite familiar with. I will just shout out on this PR if I need anything. And Friday is probably no easier for me either.

[KevinRansom]

@realvictorprm, I may also twist Brett's arm and have him look at it instead.

[dsyme]

What we could do, if we're interested in a general smoke test for various blessed OSS libraries, is set up a separate CI run that does not have a say on if a PR is green or not. That could take the built compiler and build certain projects, reporting back success/failure. I'd be in favor of having that point to latest master, since this could also be seen as a bit of a partnership between us and the library authors. It has worked very, very well with SAFE and I think it could work well here, too.

I'd be cool with this approach too.

[realvictorprm]

Ok 👍
@KevinRansom please just notify me if you need something done 😃 ! I'll take time for that in my weekly office hours. During those I'll also try to work on further SRTP bugs fixes and solutions.

[KevinRansom]

Thanks mate, I appreciate it.

[KevinRansom]

I don't think we want to add to our test-suite at this time.

[realvictorprm]

To my understanding not much will happen in near time and this part will most likely be outdated until one wants to add it in again. there for I'll close it and leave it as reference for future people who want to take over this.