Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gradual migration of fsharp and fsharpqa test suites #7075

Closed
cartermp opened this issue Jun 28, 2019 · 13 comments
Closed

Gradual migration of fsharp and fsharpqa test suites #7075

cartermp opened this issue Jun 28, 2019 · 13 comments

Comments

@cartermp
Copy link
Contributor

This is a continuation of #697 (and #872 which attempted to resolve it).

A large amount of compiler and FSharp.Core tests are run with older, slower infrastructure. They're difficult for OSS contributors to work with, it's also unclear if everything can be reported correctly in Azure CI, andt hese tests are not usable within an IDE that understands standard unit test frameowrks such as NUnit or xUnit.

As #872 showed, the path to migration is not simple. However, this doesn't mean that small chunks cannot be moved incrementally. There are two NUnit-based test projects for the compiler and FSharp.Core that are relatively easy to add and modify tests for:

Compiler:
https://github.com/dotnet/fsharp/tree/master/tests/FSharp.Compiler.UnitTests and https://github.com/dotnet/fsharp/tree/master/tests/fsharp/Compiler/Language

Includes useful utilities such as:

  • Compiling a single arbitrary string of F# code and verifying that it passes or fails
  • Comparing the emitted IL when compiling an arbitrary string of F# code with user-provided IL (for optimization testing)

FSharp.Core:
https://github.com/dotnet/fsharp/tree/master/tests/FSharp.Core.UnitTests

Includes a useful utility to verify the FSharp.Core API public API surface area.

In theory, it should be fairly straightforward to pick a small thing that's being tested under /fsharpqa and migrate that and only that to a new set of tests in one of the above NUnit test areas.

@cartermp
Copy link
Contributor Author

Related: #7076

@cartermp
Copy link
Contributor Author

@TIHan @dsyme any general thoughts? It'd be great to point to this issue and encourage interested folks in helping out - just one small area at a time.

@cartermp
Copy link
Contributor Author

Also adding @enricosada and @forki for thoughts.

@TIHan
Copy link
Contributor

TIHan commented Jun 28, 2019

My general thought, the ideal place to be is to have the FSharp.Compiler.UnitTests contain everything we need and use XUnit.

@forki
Copy link
Contributor

forki commented Jun 29, 2019

@TIHan I tried to activate Fscheck in the FSharp.Compiler.unittest but failed. Can you please give it a try? It would be really important to have PBTs on things like SuggestionBuffer

@A-Manning
Copy link

A-Manning commented Jul 3, 2019

Would really like to see progress on this, it's a serious hurdle for anyone looking to develop on linux, since the fsharpqa suite won't run.

@sergey-tihon
Copy link
Contributor

How to run tests from FSharpSuite.Tests.fsproj on macOS?
I looks like ./eng/build.sh -t does not execute this suite for some reason... Does it mean that tests are win-only?

I did naive attempt to run dotnet test FSharpSuite.Tests.fsproj but it failed
D_TBr8SW4AExxi6 jpg-large

@TIHan
Copy link
Contributor

TIHan commented Jul 12, 2019

FSharpSuite.Tests I believe will not work anything but windows with VS installed. We have some work to do.

@jrr
Copy link

jrr commented Jul 14, 2019

Can any of the to-be-migrated tests run on non-Windows?

How about after migration?

I'm a (potential) first-time contributor coming from F# Weekly, and it's not clear if I can be of much help on my Mac.

If you're looking to crowdsource this, enabling non-Windows use would unblock a lot of potential contributors!

@sergey-tihon
Copy link
Contributor

@jrr I did my PR from macOS. I did it like this

  • I moved small portion of old files to one file with Unit tests (copied expectedServerity, expectedErrorNumber and expectedErrorMsg from old files)
  • Double-checked expectedErrorMsg using .NET Core FSI (in my case error message slightly different from the old one)
  • Created Draft PR to run Azure Pipelines CI builds (including Windows)
  • Updated PR based on error messages with correct expectedErrorRange (double-checked that I understand new ranges)

Yes, it is not the easiest way, but doable.

In my case I am able to execute tests on Windows Parallels VM, so I appreciate suggestions how to run only FSharpSuite.Tests.fsproj from command line on Windows.

@nmfisher
Copy link

Not being able to run tests on Linux has obviously been a blocker for quite a few people (myself included).

Personally I think it would be great to remove this, not only for F# as a cross-platform language, but also to encourage contributions from the non-Windows community. I obviously can't speak for the maintainers though :)

Either way, I've been through the code and it's mostly typical Linux vs Windows issues - filesystem/environment variable/case sensitivity/etc.

I've added some fixes and can now run the fsharp tests (not fsharpqa, but happy to fix those too) on my Linux system.

Can anyone help check by:

  1. running this on their own Linux system
  2. running the whole suite on Windows to make sure I haven't broken anything (is there anyway to trigger this via CI?)

Repository is @ https://github.com/nmfisher/fsharp

To run tests/fsharp tests on Linux:

[projects]$ git clone -b linuxtesting git@github.com:nmfisher/fsharp.git fsharp_linuxtesting
[projects]$ cd fsharp_linuxtesting
[fsharp_linuxtesting]$ ./build.sh 
[fsharp_linuxtesting]$ cd ./tests/fsharpqa/testenv/src/PEVerify 
[PEVerify]$ ../../../../../.dotnet/dotnet build PEVerify.csproj
[PEVerify]$  cd ../../../../fsharp
[fsharp]$ ../../.dotnet/dotnet test FSharpSuite.Tests.fsproj

This is my first time contributing to F# itself, so please let me know if there's something I've missed or that I should (or should not) be doing.

@A-Manning
Copy link

@nmfisher Porting the fsharpqa suite would be great!
I ran the tests, everything worked fine for me :)

@cartermp
Copy link
Contributor Author

@vzarytovskii and friends are working towards this being a reality, I'll close in favor of the issues set to track that work

@cartermp cartermp removed this from the Backlog milestone Sep 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants