-
Notifications
You must be signed in to change notification settings - Fork 785
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
Fix for #270, and framework for testing cross-targeting scenarios #275
Closed
Closed
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
e9772de
Corrections to ',' and '+' escaping for type names in quotations
latkin bd69602
Mini test framework for validating multitargeting matrix
latkin a0dd05e
Test cases for commas in type name
latkin 24a77be
Add new tests to CI smoke suite
latkin e97a592
Added comments
latkin File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
114 changes: 114 additions & 0 deletions
114
tests/fsharpqa/Source/MultiTargeting/MultiTargetMatrix.fsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,114 @@ | ||
open System | ||
open System.IO | ||
open System.Diagnostics | ||
open System.Reflection | ||
|
||
module Helpers = | ||
|
||
// runs a program, and exits the script if nonzero exit code is encountered | ||
let private run exePath args = | ||
let args = String.concat " " args | ||
let psi = ProcessStartInfo(FileName = exePath, Arguments = args, CreateNoWindow = true, UseShellExecute = false, RedirectStandardError = true) | ||
let p = Process.Start(psi) | ||
match p.WaitForExit(10 * 60 * 1000) with | ||
| false -> eprintfn "Process timed out"; exit 1 | ||
| true -> | ||
if p.ExitCode <> 0 then | ||
eprintfn "%s %s" exePath args | ||
eprintfn "%s" (p.StandardError.ReadToEnd()) | ||
exit p.ExitCode | ||
|
||
let private authorCompile compilerPath runtime source = | ||
run compilerPath ["-a"; "-o:author.dll"; "--noframework"; sprintf "\"-r:%s\"" runtime; source] | ||
|
||
let private consumerCompile compilerPath runtime source = | ||
run compilerPath ["-o:consumer.exe"; "--noframework"; sprintf "\"-r:%s\"" runtime; "-r:author.dll"; source] | ||
|
||
let private consumerRunFsi fsiPath source = | ||
run fsiPath ["--exec"; source] | ||
|
||
// runs the consumer EXE, handling binding redirects automatically | ||
let private consumerRunExe redirectVer = | ||
if File.Exists("consumer.exe.config") then | ||
File.Delete("consumer.exe.config") | ||
|
||
let content = File.ReadAllText("consumer.exe.config.txt").Replace("{ver}", redirectVer) | ||
File.WriteAllText("consumer.exe.config", content) | ||
|
||
run "consumer.exe" [] | ||
|
||
/// gets the version of the assembly at the specified path | ||
let getVer dllPath = | ||
let asm = Assembly.ReflectionOnlyLoadFrom(dllPath) | ||
asm.GetName().Version.ToString() | ||
|
||
/// runs through the end-to-end scenario of | ||
/// - Author uses [authorComiler] to build DLL targeting [authorRuntime] with source [authorSource] | ||
/// - Consumer uses [consumerCompiler] to build EXE ref'ing above DLL, building EXE targeting [consumerRuntime] with source [consumerSource] | ||
/// - Run the resulting EXE | ||
let testExe authorCompiler authorRuntime consumerCompiler consumerRuntime authorSource consumerSource = | ||
authorCompile authorCompiler authorRuntime authorSource | ||
consumerCompile consumerCompiler consumerRuntime consumerSource | ||
consumerRunExe (getVer consumerRuntime) | ||
|
||
/// runs through the end-to-end scenario of | ||
/// - Author uses [authorComiler] to build DLL targeting [authorRuntime] with source [authorSource] | ||
/// - Consumer uses [consumerFsi] to #r above DLL and run script [consumerSource] | ||
let testFsi authorCompiler authorRuntime consumerFsi authorSource consumerSource = | ||
authorCompile authorCompiler authorRuntime authorSource | ||
consumerRunFsi consumerFsi consumerSource | ||
|
||
module Test = | ||
let private env s = | ||
match Environment.GetEnvironmentVariable(s) with | ||
| var when not (String.IsNullOrWhiteSpace(var)) -> var | ||
| _ -> failwithf "Required env var %s not defined" s | ||
|
||
// paths to vPrevious of fsc.exe, fsi.exe, FSharp.Core.dll | ||
let vPrevCompiler = env "FSCVPREV" | ||
let vPrevFsi = Path.Combine(env "FSCVPREVBINPATH", "fsi.exe") | ||
let vPrevRuntime = env "FSCOREDLLVPREVPATH" | ||
|
||
// paths to vCurrent of fsc.exe, fsi.exe, FSharp.Core.dll | ||
let vCurrentCompiler = env "FSC" | ||
let vCurrentFsi = Path.Combine(env "FSCBINPATH", "fsi.exe") | ||
let vCurrentRuntime = env "FSCOREDLLPATH" | ||
|
||
let cases = | ||
// compiler/runtime of author | compiler/runtime of consumer | ||
[ 0, Helpers.testExe vPrevCompiler vPrevRuntime vCurrentCompiler vPrevRuntime | ||
1, Helpers.testExe vPrevCompiler vPrevRuntime vCurrentCompiler vCurrentRuntime | ||
2, Helpers.testExe vCurrentCompiler vPrevRuntime vPrevCompiler vPrevRuntime | ||
3, Helpers.testExe vCurrentCompiler vPrevRuntime vCurrentCompiler vPrevRuntime | ||
4, Helpers.testExe vCurrentCompiler vPrevRuntime vCurrentCompiler vCurrentRuntime | ||
5, Helpers.testExe vCurrentCompiler vCurrentRuntime vCurrentCompiler vCurrentRuntime | ||
|
||
// compiler/runtime of author | fsi of consumer | ||
6, Helpers.testFsi vPrevCompiler vPrevRuntime vCurrentFsi | ||
7, Helpers.testFsi vCurrentCompiler vPrevRuntime vCurrentFsi | ||
8, Helpers.testFsi vCurrentCompiler vPrevRuntime vPrevFsi | ||
9, Helpers.testFsi vCurrentCompiler vCurrentRuntime vCurrentFsi | ||
] | ||
|
||
// parse command line args | ||
// final 'exclusions' arg allows for certain scenarios to be skipped if they are not expected to work | ||
let authorSource, consumerSource, exclusions = | ||
match fsi.CommandLineArgs with | ||
| [| _; arg1; arg2 |] -> arg1, arg2, [| |] | ||
| [| _; arg1; arg2; arg3 |] -> arg1, arg2, (arg3.Split(',') |> Array.map int) | ||
| args -> | ||
eprintfn "Expecting args <author source> <consumer source> [excluded cases], got args %A" args | ||
exit 1 | ||
|
||
// failsafe to make sure that excluded scenarios are revisited on new versions | ||
// i.e. exclusions valid for vN/vN-1 will probably no longer be needed for vN+1/vN | ||
if not ((Helpers.getVer Test.vCurrentRuntime).StartsWith("4.4.0")) then | ||
eprintfn "Runtime version has changed, review exclusions lists for these tests" | ||
exit 1 | ||
|
||
Test.cases | ||
|> List.filter (fun (id, _) -> not (Array.contains id exclusions)) | ||
|> List.iter (fun (id, testCase) -> | ||
printfn "Case %d" id | ||
testCase authorSource consumerSource | ||
) |
5 changes: 5 additions & 0 deletions
5
tests/fsharpqa/Source/MultiTargeting/QuotedCommaTypeName_author.fs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
module Foo | ||
|
||
type ``one, two, three``() = class end | ||
|
||
let X = <@ ``one, two, three``() @> |
11 changes: 11 additions & 0 deletions
11
tests/fsharpqa/Source/MultiTargeting/QuotedCommaTypeName_consumer.fsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
#if INTERACTIVE | ||
#r "author.dll" | ||
#else | ||
module Test | ||
#endif | ||
|
||
printfn "%A" Foo.X | ||
|
||
#if INTERACTIVE | ||
#q ;; | ||
#endif |
16 changes: 16 additions & 0 deletions
16
tests/fsharpqa/Source/MultiTargeting/consumer.exe.config.txt
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
<?xml version="1.0" encoding="utf-8"?> | ||
<configuration> | ||
<runtime> | ||
<assemblyBinding xmlns="urn:schemas-microsoft-com:asm.v1"> | ||
<dependentAssembly> | ||
<assemblyIdentity | ||
name="FSharp.Core" | ||
publicKeyToken="b03f5f7f11d50a3a" | ||
culture="neutral"/> | ||
<bindingRedirect | ||
oldVersion="2.0.0.0-{ver}" | ||
newVersion="{ver}"/> | ||
</dependentAssembly> | ||
</assemblyBinding> | ||
</runtime> | ||
</configuration> |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
// Every testcase need a sourcefile, so this is a fake one. | ||
// Add some code so that you don't get the 'empty module' warning. | ||
|
||
if false then | ||
printfn "Hello, World!" | ||
|
||
exit 0 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
As far as I can tell from my testing, this comment is just wrong -- I can't find any scenarios where the + is escaped. Indeed, the incorrect escaping needed to be removed in order to get my tests passing. All current tests continue to work fine.
This is now simplified and exactly matches https://github.com/Microsoft/visualfsharp/blob/fsharp4/src/absil/il.fs#L682
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.
That code had always been very, very suspect to me. Thanks for digging into this so thoroughly