The JaroWinkler tests fail on different locales

[abelbraaksma]

There are four JaroWinkler distance tests. These fail when run on a computer with a different locale.

Repro steps

Run the tests under FSharp.Compiler.Unittests.EditDistance on a German, Dutch or basically any non-US/UK computer and you will get four failing tests, because the output uses ToString internally without taking care of the locale.

Suggested fix: add Culture.InvariantCulture.

Expected behavior

Any unit test should run successfully irrespective of the environments locale settings.

Actual behavior

The following error is thrown:

  String lengths are both 5. Strings differ at index 1.
  Expected: "0.813"
  But was:  "0,813"
  ------------^

   at NUnit.Framework.Internal.Commands.TestMethodCommand.Execute(TestExecutionContext context)
   at NUnit.Framework.Internal.Commands.TestActionCommand.Execute(TestExecutionContext context)

(this was one of the findings after starting investigating test coverage, see #3579)

[forki]

Feel free to fix it ;-) Am 14.09.2017 7:31 nachm. schrieb "Abel Braaksma" <notifications@github.com

…

: There are four JaroWinkler distance tests. These fail when run on a computer with a different locale. Repro steps Run the tests under FSharp.Compiler.Unittests.EditDistance on a German, Dutch or basically any non-US/UK computer and you will get four failing tests, because the output uses ToString internally without taking care of the locale. Suggested fix: add Culture.InvariantCulture. Expected behavior Any unit test should run successfully irrespective of the environments locale settings. Actual behavior The following error is thrown: String lengths are both 5. Strings differ at index 1. Expected: "0.813" But was: "0,813" ------------^ at NUnit.Framework.Internal.Commands.TestMethodCommand.Execute(TestExecutionContext context) at NUnit.Framework.Internal.Commands.TestActionCommand.Execute(TestExecutionContext context) (this was on the findings after starting investigating test coverage, see #9579) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#3583>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADgNAcZMg4GPNn5z1DcIgeVvKip_YO2ks5siWL9gaJpZM4PX7Ui> .

[abelbraaksma]

@forki, I'm planning on doing that :)

[enricosada]

@abelbraaksma there is an helper to require locale us in tests, this is an issue for other tests too

[abelbraaksma]

A fix is now in PR #3627.

[KevinRansom]

@abelbraaksma there is no harm in fixing this.

[abelbraaksma]

@KevinRansom, sorry, I think I misunderstood the process. I thought you approved my changes in PR #3627, hence I closed it. Guess that was too early? I see it now mentions "closed with unmerged commits". Sorry, first time PR to this repo... I'll reopen the PR.