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

Enable more tests on CI #332

Merged
merged 7 commits into from Feb 14, 2019
Merged

Conversation

baronfel
Copy link
Contributor

This PR enables a few tests what were blocked off, as well as converting the msbuild-4 tests to use toolsversion 15 and adding in explicit fsharp.core and system.runtime references as appropriate (ie. when the test wasn't explicitly testing a missing fsharp.core reference).

This is currently failing across most of the cloud providers, but I wanted to open it up as a base for discussion and ensure that I was thinking about things the correct way.

@enricosada
Copy link
Contributor

seems ok to me.

dunno why fsharp.core doesnt resolve from GAC in mono, but who care atm, is ok with HintPath

@enricosada
Copy link
Contributor

@baronfel i think some errors are because MSbuild 12 is not installed in appveyor.
let me fix that

This was referenced Feb 13, 2019
@enricosada enricosada self-assigned this Feb 13, 2019
@baronfel
Copy link
Contributor Author

Looks like on appveyor we need to add fsharp.core and system.io references to a few old-style project files, and on linux we've got much larger problems:

Failed to parse file: <absolute path removed>/FsAutoComplete.IntegrationTests/RobustCommands/Project/FileTwo.fs
Failed to parse file: /home/travis/build/fsharp/FsAutoComplete/test/FsAutoComplete.IntegrationTests/RobustCommands/Project/Program.fs

those aren't even in json blobs, those are coming from the background file parser if I'm reading this right

baronfel and others added 2 commits February 14, 2019 01:01
- reenable outputcache and invalidprojectfile testscases
- update the output.json for each of these cases was consistent with a) the changed stacktraces of the ProjectCrackerTool and b) the 'quitting...' info message
- fix regexes that were over-escaping the groups/characters at the end
- readd the linter test exceptions
- update errors test project file to version 15
- add system.runtime reference
- strip packagesdir paths

reenable tests:

- finddeclarations
- multiproj
- multiunsaved
- paramcompletion
- projectreload
- robustcommands
- symboluse
- test1json
- nofsharpcore
- uncompiledreferencedprojects

adjust baselines:
- for new information coming back from symboluse
- because there's a file parse notification now

update expected message from CI run
that's require to install MSBuild v12 assemblies in GAC, needed by FCS
@enricosada
Copy link
Contributor

enricosada commented Feb 14, 2019

@baronfel i rebased and squashed the commits.

i fixed the projects, in win should be ok.

the TargetFrameworkVersion property was missing, and so it was using net40 as default.
The fsharp.core was not loading because compiled with net45.
Running msbuild /t:Build of projects shown the warning, and the new dotnet-proj-info project parser was working ok (same results as msbuild), instead project cracker was trying to fix things

@baronfel i'll check ci for travis

@baronfel
Copy link
Contributor Author

Nice detective work!

must specify the target framework, otherwise is v4.0 by default.

the project wasnt building either (`msbuild /t:Build`) with warning

```
  C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\MSBuild\15.0\bin\Microsoft.Common.CurrentVersion.targe
ts(2106,5): warning MSB3274: The primary reference "FSharp.Core" could not be resolved because it was built against the
 ".NETFramework,Version=v4.5" framework. This is a higher version than the currently targeted framework ".NETFramework,
Version=v4.0". [e:\github\fsharp\FsAutoComplete\test\FsAutoComplete.IntegrationTests\ErrorTestsJson\Test1.fsproj]
  C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\MSBuild\15.0\bin\Microsoft.Common.CurrentVersion.targe
ts(2106,5): warning MSB3267: The primary reference "System.IO", which is a framework assembly, could not be resolved in
 the currently targeted framework. ".NETFramework,Version=v4.0". To resolve this problem, either remove the reference "
System.IO" or retarget your application to a framework version which contains "System.IO". [e:\github\fsharp\FsAutoComp
lete\test\FsAutoComplete.IntegrationTests\ErrorTestsJson\Test1.fsproj]
```

remove useless `System.Runtime` to minimize changes
@enricosada
Copy link
Contributor

enricosada commented Feb 14, 2019

appveyor is now green.

the travis error is strange, i'll print the expection message too.
if no clues, i'll disable the test on unix and merge it

@enricosada enricosada changed the title WIP: Enable more tests on CI Enable more tests on CI Feb 14, 2019
@enricosada
Copy link
Contributor

enricosada commented Feb 14, 2019

ok, on mono a ThreadAbortException is raised.

Dunno if that's because is not killed fast enough or another reason, but can be ignored i think

@baronfel
Copy link
Contributor Author

It looks like the base error message coming back from MSBuild is being normalized in different ways on windows and *nix.

@baronfel
Copy link
Contributor Author

Yeah it looks like on mono now we'll need to strip off the in [MEMORY LOCATION]:[LINE NUMBER] part on the mono newlines as part of the normalization now.

@baronfel
Copy link
Contributor Author

saved:

Microsoft.FSharp.Compiler.SourceCodeServices.ProjectCrackerTool.ProjectCrackerTool.FSharpProjectFileInfo..ctor(String fsprojFileName, FSharpOption`1 properties, FSharpOption`1 enableLogging)\n   at 

different:

Microsoft.FSharp.Compiler.SourceCodeServices.ProjectCrackerTool.ProjectCrackerTool+FSharpProjectFileInfo..ctor (System.String fsprojFileName, Microsoft.FSharp.Core.FSharpOption`1[T] properties, Microsoft.FSharp.Core.FSharpOption`1[T] enableLogging) [0x000ba] in <5b8fc032f73f2f25a745038332c08f5b>:0 \n  at 

differences I see:

  • type names are fully-qualified
  • memory location and line number are present now

@enricosada
Copy link
Contributor

enricosada commented Feb 14, 2019

yes. i'll disable for now on mono.
also the message is not exactly the same, so unless we rewrite the message completly, is not the same, but ihmo it's just a minor diff.

@baronfel
Copy link
Contributor Author

tracking the normalization issues in #339

@enricosada
Copy link
Contributor

naa, it doesnt matter that much. tests like that are few.

the text can be normalized or the message rewritten, and will be ok.

@baronfel
Copy link
Contributor Author

The test failure now is coming from the project type parsing, which is reading from a streamreader. Somehow the streamreader line is null? That's weird.

@Krzysztof-Cieslak
Copy link
Member

Green!

@enricosada enricosada merged commit 8ecca81 into ionide:master Feb 14, 2019
@enricosada
Copy link
Contributor

Fix it, one commit at time.

Awesome work @baronfel

@baronfel baronfel deleted the enable_more_tests branch October 21, 2021 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants