-
Notifications
You must be signed in to change notification settings - Fork 805
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
Migrate a set of fsharpqa tests to new test suite (overload resolution error messages) #6654
Migrate a set of fsharpqa tests to new test suite (overload resolution error messages) #6654
Conversation
…cted by dotnet#6596 the baseline files are empty on purpose, expecting CI failure reporting those tests, I intend to update the baseline and clean up the comments / xml tags from fsharpqa syntax in later commit, and then remove those specific tests altogether from fsharpqa if this is OK with the maintainers.
Master fsharpqa: https://dev.azure.com/dnceng/public/_build/results?buildId=167958
this PR:
-12 expected Master Windows desktop_release: https://dev.azure.com/dnceng/public/_build/results?buildId=173798
this PR:
+12 expected This is ready |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, anything that reduces our reliance on fsharpqa is good with me.
Thanks for the feedback @KevinRansom, I wasn't able to run the migrated tests successfully under .netcore + failure on CI so I disabled them with the #define used in the tests.fs for now. the error where it failed:
https://dev.azure.com/dnceng/public/_build/results?buildId=173542 Maybe you'll know better if we can fix this and enable them on core compiler target. I'm also wondering about the status of giving .fsx files to fsc.exe, the migrated fsharpqa tests are under .fsx for tooling reason (opening the file shows the errors, not if it is a .fs). I’ve migrated that exact set of tests: I double checked it in the .lst changes, but it would be great to have it cross checked as well. |
I was just telling @brettfo, I would see what it took to make the singlenegtest handle coreclr as a weekend, fun/misery. Project before long.
|
Thanks @brettfo there we go, -12 fsharpqa tests, +12 potentially multi target and easier to maintain tests 🙂. |
@brettfo @KevinRansom would you happen to know why the .bsl (& .err) files have redundant CR compared to textual output from the compiler? It would be worth fixing it if we know the reason and it is not a big change (the diff in .bsl files will be big though). |
I think it's best to treat those tests as a big blob of "Here there be dragons" … and cautiously migrate out the tests. However, there is a big problem with doing that … the fsharpqa tests can use the F# compiler server, so we don't constantly spin up a new compiler for the tests that need compilation. the Cambridge tests don't do that. So migrating them earns us a slow down. too much to do. |
This PR intends to migrate fsharpqa tests that are going to be impacted by #6596.
My plan is to be able to rebase the other PR once those tests from fsharpqa have been migrated to new suite.
Adjusting fsharpqa tests for multiline error message is too much effort, and it would save me some effort if this PR is pertinent.
Note: I'm using .fsx files for the tests, the main reason is that the tooling picks up errors on the spot even if the files aren't loaded from a project; I can't get this behaviour to work in VS2019 with .fs files.