Adding the Runtime Identifier and PlatformTarget to the error message

[livarcocc]

Adding the Runtime Identifier and PlatformTarget to the error message for when they don't match.

Fixes #1616

[livarcocc]

@nguerrera is this OK for the loc CI?

[peterhuene]

Could we not string.Format using Strings.CannotHaveRuntimeIdentifierPlatformMismatchPlatformTarget?

[peterhuene]

Nit: double semicolons at end of line.

[wli3]

We had discussion on loc with tests. It is bad we assert string.format with string.format but it is too hard to run these tests on the loc CI (CI machine in different language). cc @nguerrera

[peterhuene]

I (extremely weakly to the point of not really having any opinion on this) disagree that it's bad to assert against a formatted string; the test should verify the output is the expected resource string, whatever it is. The test can't catch grammatical or formatting errors in the string; those can only be caught by human review, which is done on the resource files anyway.

Still, I can see how it'd be a good thing that the test breaks when the resource changes; another opportunity to review the string I suppose.

On the negative side, however, the test with a hardcoded expected value won't catch a failure to localize the string because we would have to skip it on localized builds.

[wli3]

The problem is this test will fail on a CI that has a different language enabled if we keep it that way. Maybe an attribute of skip on localization build is also fine.

[livarcocc]

I noticed that on the CLI, we use string.Format and work with the Resx resource file. I think this is what I will do here as well. Just need to figure out if I want to make the Strings class public so that the test can see it (I think I prefer this) or use a InternalsVisibleTo attribute.

[nguerrera]

There are a lot of trade-offs here. We should never skip an entire test on a localized setup unless the only purpose of the test is to assert the value of a string. Generally, there are other assertions that are more directly under test and we should retain coverage of those on non-English. Consider cases where an issue with localization causes a behavior change other than translation. Clasically, a translation with incorrect placeholders could cause a formatting exception / crash.

In the CLI repo, I did a mix of these two things:

(1)

operation.Should().Succeed(); // we always want to check this and not skip the entire test
message.Should().BeIfNotLocalized("Something 42"); 

(2)

operation.Should().Succed();
message.Should().Be(string.Format(Strings.Something, 42))"

A third (simpler) approach, which might be good enough here, would be to just check for the placeholder args: assert that message contains both win10-x64 and x86.

I chose (1) where I didn't think hard-coding a copy of the English string was adding enough value, and (2) where I thought it was. One example where significant value was added was the complex formatting of multiple strings that ended up as the help text for a command.

BeIfNotLocalized was a custom assertion that I added to avoid having branches all over the tests. It could be generalized to something that takes (string formattedString, string format, params string[] args) and asserts that formattedString is the exact string on English and that string.Format(format, args) is the string on all locales. This would maximize coverage, but make the tests more difficult to write and maintain. Again, trade-offs.