-
Notifications
You must be signed in to change notification settings - Fork 124
Dotnet Core tests - wip #561
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
Conversation
| "../service/FscTests.fs", | ||
| "../service/ProjectOptionsTests.fs", | ||
| "../service/PerfTests.fs", | ||
| "../service/ExprTests.fs", |
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.
Of these, I think ExprTests should likely pass relatively easily?
|
This looks really good, and great to see it is green already (although I'm not sure if netcore build + test is running in CI yet?) |
|
@dsyme No, not running the netcore test target in CI yet, it needs to be added as a different platform in the appveyor matrix, so it doesn't impact the other tests. |
|
@dsyme I added the dotnet core tests to the build matrix in appveyor so we can see which tests are failing. The dotnet core test target is currently ignored if it fails, as there are a lot of tests that still need to be fixed. I couldn't find a way to show the individual build job status separately (on the same branch), but you can click on the latest appveyor build and see status there. If anything catches your eye on a particular failing test as an easy fix, any advice will be highly appreciated. I don't have a working debug setup (still waiting for rc2 tooling) and the only way to debug right now is with print statements, so progress is slow. If you have a better debug setup, please share. |
Use FSharp.Compiler.Tools instead of bootstrap
|
@dsyme All included tests now passing on netcore RC2. |
| @@ -0,0 +1,361 @@ | |||
| namespace Microsoft.FSharp.Compiler.SourceCodeServices.ProjectCrackerTool | |||
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.
This looks more or less duplicated from https://github.com/fsharp/FSharp.Compiler.Service/blob/340b9ab6ca1dd90ab7fcd89d503acc4553a3bca9/src/fsharp/FSharp.Compiler.Service.ProjectCrackerTool/Program.fs. Do you think we can avoid that duplication?
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.
This duplication is intentional to minimize the impact on other projects, as it is a small refactoring (breaking the above mentioned Program.fs in two files so it can be reused), but yes, I can definitely do that, either here or in a subsequent PR.
|
This looks great. I suppose there may be further change in this area ahead given this announcement https://blogs.msdn.microsoft.com/dotnet/2016/05/27/making-it-easier-to-port-to-net-core/ and the other changes in project.json. But this looks right at this stage. For example, I suspect we might be able to use MSBuild and paket for the build in due course given the drift back to the XML-based project file tooling. But we can deal with that in another iteration. Do you think you could update this with the necessary changes to make sure the FSharp.Compiler.Service.dll is included in the NuGet package (presumably as a .NETStandard 1.5 profile component?) |
|
(If you want to do the NuGet package additions in a separate PR that's also OK, I think we can merge this as is) |
|
Another PR would probably be better, to get some feedback on the versioning. The netcore build already copies the nuget packages into the top bin folder, so they are available as artifacts in appveyor. |
|
Also to be addressed in a subsequent PR (I'll open an issue), there is still an outstanding assembly resolution problem with checker.GetProjectOptionsFromScript(file, code), similar to #535 and #567, that may need to be fixed before pushing an official netcore version to NuGet. Tests are currently avoiding it (netcore only) by not using it: |
|
OK, I will merge this then. |
Initial setup, work in progress. Some of the tests (but not all) are failing on dotnet core being different, others are not yet feasible due to dependency on ProjectCracker, etc.
Comments / advise is welcome.