Validate product resource strings against resx files #4811

JamesWTruher · 2017-09-11T22:12:08Z

These tests validate that the resource strings represent the contents of the RESX files. This essentially ensures that we've run -ResGen when building. Additionally, these tests will ensure that we don't have ResourceIds which have a space in them (which we did).

When there's a space in the ResourceId, the tooling converts the space to an "_" which is fine, but the compiled resource doesn't have that change, so when message retrieval is attempted, an empty string is returned.

Normally we generate an error and validate the ErrorId, but this test is different. We want to be sure that the message that we're giving the user is correct and the build included the -ResGen parameter.

iSazonov · 2017-09-12T03:18:59Z

Does it make sense to check for unused ResourceId in resx files?

JamesWTruher · 2017-09-12T21:27:46Z

@iSazonov re: check for unused ResourceId in resx files
it would be good to check for these. I thought about how to do that, but couldn't come up something simple, other than brute force checking all the source files.

SteveL-MSFT · 2017-09-13T00:01:08Z

test/powershell/engine/ResourceValidation/TestRunner.ps1

@@ -0,0 +1,57 @@
+function Test-ResourceStrings


Would it be better to put this into an existing module or a new test module?

i wanted to do it this way because I do not want to make this a general function - we shouldn't be testing internal data structures, but there's no way to do this test without it.

SteveL-MSFT · 2017-09-13T00:03:53Z

test/powershell/engine/ResourceValidation/ConsoleHostResources.Tests.ps1

+$AssemblyName = "Microsoft.PowerShell.ConsoleHost"
+
+# excluded resources
+$excludeList = @("HostMshSnapinResources.resx")


Should add comment why it's being excluded

SteveL-MSFT · 2017-09-13T00:04:43Z

test/powershell/engine/ResourceValidation/MPCUResource.Tests.ps1

+$AssemblyName = "Microsoft.PowerShell.Commands.Utility"
+# this list is taken from ${AssemblyName}.csproj
+# excluded resources
+$excludeList = "CoreMshSnapinResources.resx",


Add comment why these are excluded

SteveL-MSFT · 2017-09-13T00:05:33Z

test/powershell/engine/ResourceValidation/ManagementCommandsResources.Tests.ps1

+$AssemblyName = "Microsoft.PowerShell.Commands.Management"
+# this list is taken from ${AssemblyName}.csproj
+# excluded resources
+$excludeList = "EventlogResources.resx",


Added comment why excluded

SteveL-MSFT · 2017-09-13T00:05:45Z

test/powershell/engine/ResourceValidation/SMAResource.Tests.ps1

+$AssemblyName = "System.Management.Automation"
+# this list is taken from ${AssemblyName}.csproj
+# excluded resources
+$excludeList = "CoreMshSnapinResources.resx",


Comment why excluded

SteveL-MSFT · 2017-09-13T00:05:58Z

test/powershell/engine/ResourceValidation/SecurityResource.Tests.ps1

+$assemblyName = "Microsoft.PowerShell.Security"
+# this list is taken from ${AssemblyName}.csproj
+# excluded resources
+$excludeList = @("SecurityMshSnapinResources.resx")


Comment why excluded

SteveL-MSFT · 2017-09-13T00:09:48Z

test/powershell/engine/ResourceValidation/TestRunner.ps1

+
+    # determine the needed resource directory. If these tests are moved
+    # this logic will need to change
+    $repoBase = (Resolve-Path (Join-Path $psScriptRoot ../../../..)).Path


Is it better to use: git rev-parse --show-toplevel?

sure - I'll make that change

Cannot assume that we have git tools. For example, the Code Coverage machine does not have git tools.

in that case, this is fine

SteveL-MSFT · 2017-09-13T00:10:37Z

test/powershell/engine/ResourceValidation/TestRunner.ps1

+    $repoBase = (Resolve-Path (Join-Path $psScriptRoot ../../../..)).Path
+    $asmBase = Join-Path $repoBase "src/$AssemblyName"
+    $resourceDir = Join-Path $asmBase resources
+    $resourceFiles = Get-ChildItem $resourceDir -Filter *.resx -ea stop | Where-Object {


Why not use -Exclude parameter for Get-ChildItem?

i can do that

huh - it turns out that we might have a bug - get-childitem -filter *.resx -exclude <thing> returns no results. I'll have to leave it as is.

Now that you mention it, there's a number of issues with -exclude. Ok with leaving as-is

daxian-dbw

Leave 2 comments. They don't block this PR.

daxian-dbw · 2017-09-13T01:13:58Z

test/powershell/engine/ResourceValidation/DotNetEventing.Tests.ps1

@@ -0,0 +1,10 @@
+. "$psscriptroot/TestRunner.ps1"


To be consistent with other files, maybe change the file name from DotNetEventing.Tests.ps1 to ***Resource.Tests.ps1

will change

daxian-dbw · 2017-09-13T01:15:16Z

test/powershell/engine/ResourceValidation/MPCUResource.Tests.ps1

@@ -0,0 +1,17 @@
+. "$psscriptroot/TestRunner.ps1"


Since the file for management dll is ManagementCommandsResources.Tests.ps1, Maybe change MPCUResource.Tests.ps1 to Utility.Resource.Tests.ps1?

will change

adityapatwardhan

LGTM

…mbly this will validate that the messages that we emit match the resource file unsync can occur when not compiling with -ResGen, this will keep that from happening. A side effect of these tests is that it will catch resource ids which have spaces in them

Also rename some files for better consistency

JamesWTruher · 2017-09-14T22:37:14Z

@SteveL-MSFT updated and pushed - see my comment about using -filter and -exclude together - I double checked with PowerShell 5 and the behavior is the same. if you use -filter and -exclude no results are returned, so at least it's not a regression, although i agree the behavior is a little surprising

iSazonov · 2017-09-15T03:35:01Z

@JamesWTruher re: check for unused ResourceId in resx files

It seems @lzybkr once mentioned some tool.
As for me I think better way is to get native warning from the compiler that does still not implemented.

lzybkr · 2017-09-18T00:04:07Z

@iSazonov - I use ReSharper to find unused references. There were some accuracy issues when the csproj file wasn't the same as how we built, but it worked fairly well anyway, so I'm guessing it's pretty accurate now.

iSazonov · 2017-09-18T07:53:00Z

@lzybkr Thanks!
I see there is free ReSharper command tools

JamesWTruher · 2017-09-18T17:35:22Z

@iSazonov @lzybkr I created #4860 to track

JamesWTruher assigned TravisEz13 Sep 11, 2017

JamesWTruher requested review from SteveL-MSFT and adityapatwardhan September 11, 2017 22:12

JamesWTruher requested a review from daxian-dbw as a code owner September 11, 2017 22:12

msftclas added the cla-not-required label Sep 11, 2017

SteveL-MSFT requested changes Sep 13, 2017

View reviewed changes

daxian-dbw approved these changes Sep 13, 2017

View reviewed changes

adityapatwardhan approved these changes Sep 14, 2017

View reviewed changes

JamesWTruher added 4 commits September 14, 2017 15:05

Remove spaces which were used in the resource id for one message

b41cbeb

Refactor tests to use common code and fill in remaining gaps

8e41f3e

Add explanations as to why some resource files are skipped

0d7034a

Also rename some files for better consistency

JamesWTruher force-pushed the jamestruher/smaresourcestring branch from 46e2f48 to 0d7034a Compare September 14, 2017 22:08

SteveL-MSFT approved these changes Sep 14, 2017

View reviewed changes

TravisEz13 merged commit 0229451 into PowerShell:master Sep 18, 2017

SteveL-MSFT mentioned this pull request Sep 18, 2017

Added WSMan Config provider tests #4756

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate product resource strings against resx files #4811

Validate product resource strings against resx files #4811

JamesWTruher commented Sep 11, 2017

iSazonov commented Sep 12, 2017

JamesWTruher commented Sep 12, 2017

SteveL-MSFT Sep 13, 2017

JamesWTruher Sep 14, 2017

SteveL-MSFT Sep 14, 2017

SteveL-MSFT Sep 13, 2017

SteveL-MSFT Sep 13, 2017

SteveL-MSFT Sep 13, 2017

SteveL-MSFT Sep 13, 2017

SteveL-MSFT Sep 13, 2017

SteveL-MSFT Sep 13, 2017

JamesWTruher Sep 14, 2017

adityapatwardhan Sep 14, 2017

SteveL-MSFT Sep 14, 2017

SteveL-MSFT Sep 13, 2017

JamesWTruher Sep 14, 2017

JamesWTruher Sep 14, 2017

SteveL-MSFT Sep 14, 2017

daxian-dbw left a comment

daxian-dbw Sep 13, 2017

JamesWTruher Sep 14, 2017

daxian-dbw Sep 13, 2017

JamesWTruher Sep 14, 2017

adityapatwardhan left a comment

JamesWTruher commented Sep 14, 2017

iSazonov commented Sep 15, 2017

lzybkr commented Sep 18, 2017

iSazonov commented Sep 18, 2017

JamesWTruher commented Sep 18, 2017

Validate product resource strings against resx files #4811

Validate product resource strings against resx files #4811

Conversation

JamesWTruher commented Sep 11, 2017

iSazonov commented Sep 12, 2017

JamesWTruher commented Sep 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxian-dbw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adityapatwardhan left a comment

Choose a reason for hiding this comment

JamesWTruher commented Sep 14, 2017

iSazonov commented Sep 15, 2017

lzybkr commented Sep 18, 2017

iSazonov commented Sep 18, 2017

JamesWTruher commented Sep 18, 2017