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

Smal fixes for https://github.com/Microsoft/visualfsharp/pull/5940 #5943

Merged
merged 2 commits into from
Nov 30, 2018

Conversation

AviAvni
Copy link
Contributor

@AviAvni AviAvni commented Nov 20, 2018

Small fixes according to @blumu review

@cartermp
Copy link
Contributor

Issues:

2018-11-20T21:52:20.8974046Z Errors and Failures
2018-11-20T21:52:20.8974076Z 
2018-11-20T21:52:20.8974124Z 1) Failed : Tests.Service.Editor.Printf specifiers for triple-quote strings
2018-11-20T21:52:20.8974180Z   Expected: [|(2, 19, 2, 22, 1); (4, 12, 4, 15, 1); (6, 29, 6, 32, 1); (7, 29, 7, 31, 1);
2018-11-20T21:52:20.8974342Z   (7, 33, 7, 35, 1)|]
2018-11-20T21:52:20.8974396Z Actual: [|(2, 19, 2, 22, 1); (4, 12, 4, 15, 1); (6, 27, 6, 30, 1); (7, 29, 7, 31, 1);
2018-11-20T21:52:20.8974444Z   (7, 33, 7, 35, 1)|]
2018-11-20T21:52:20.8974533Z   Expected and actual are both <System.Tuple`5[System.Int32,System.Int32,System.Int32,System.Int32,System.Int32][5]>
2018-11-20T21:52:20.8974586Z   Values differ at index [2]
2018-11-20T21:52:20.8974633Z   Expected: <(6, 29, 6, 32, 1)>
2018-11-20T21:52:20.8974718Z   But was:  <(6, 27, 6, 30, 1)>
2018-11-20T21:52:20.8974766Z at FsUnit.shouldEqual[a](a x, a y)
2018-11-20T21:52:20.8974818Z at Tests.Service.Editor.Printf specifiers for triple-quote strings()

Looks like the same failing test in other CI runs.

Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @AviAvni

@cartermp
Copy link
Contributor

@dsyme the code for this doesn't appear to have a test for correctness. Any ideas on adding a regression test for this?

src/fsharp/NameResolution.fs Show resolved Hide resolved
@KevinRansom KevinRansom merged commit a7acf9f into dotnet:master Nov 30, 2018
@dsyme
Copy link
Contributor

dsyme commented Nov 30, 2018

IIRC this code was added to FCS and there were normally tests added for everything we did there. So the FCS tests would be the right place

dsyme pushed a commit that referenced this pull request Dec 5, 2018
* Reduce memory allocations (#5965)

* reduce memory allocations

* remove memory allocations

* Smal fixes for #5940 (#5943)

* small fixes

* fix test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants