Make ResourceManagerCache check for alternative resource paths by anmenaga · Pull Request #4139 · PowerShell/PowerShell · GitHub

This PR is for 2 things:

From WindowsPS to PSCore resource paths have changed like this example:
WindowsPS: FileSystemProviderStrings
PSCore: System.Management.Automation.resources.FileSystemProviderStrings
... and some existing modules use 'WindowsPS' syntax in their "format.ps1xml" files; for example:
<Text AssemblyName="System.Management.Automation" BaseName="FileSystemProviderStrings" ResourceId="DirectoryDisplayGrouping"/>
So in these cases resource is not found and Import-Module/Update-FormatData returns error on PSCore.
The fix is to check for alternative resource path if original fails.
when a resource is not found, currently error message has wrong assembly path which was tried for loading the resource; for example:
... resource NonExistingResource in assembly C:\System.Management.Automation is not found.
This is rather confusing, specifically because assembly, containing the resource, can be located anywhere in the filesystem. The fix makes error message accurate: ... resource NonExistingResource in assembly C:\GitHub\PowerShell\src\powershell-win-core\bin\Debug\netcoreapp2.0\win10-x64\publish\System.Management.Automation.dll is not found.

Results on PSCore after the fix:

Results on PSCore before the fix:

Do not concatenate strings, use a StringBuilder.

A single call to string.Concat with 4 or fewer arguments could be better than using a StringBuilder - the length of the string can be pre-calculated, so there would only be 1 allocation instead of multiple for the StringBuilder, the array of characters in StringBuilder, and the final string allocation.

Updated to string.Concat

Missing newline

I think the test should not look anything like something that is built-in - otherwise it might cause problems with other tests.

added newline;
renamed the format file to indicate relation to tests;
these tests are run in separate PowerShell/Runspace instances, so there should be no conflict with other tests.

Today there is no conflict.

You should write the test so there can never be a conflict, even if the code is reused.

It's really simple to use a type name specific to this scenario, so I see no reason to not do as I've suggested.

Can you do:

$_.Exception.Message | Should Match $smaLocation

this fails with
parsing 'C:\Users\GitHub\PowerShell\New\src\powershell-win-core\bin\Debug\netcoreapp2.0\win10-x64\publish\System.Management.Automation.dll' - Unrecognized escape sequence \\U.

MatchExactly fails as well.

Missing newline

updated

Can we add a test where we lookup are string resource which does not exist and expect an MissingManifestResourceException

this last test is for when resource is not found;
I've added check for FullyQualifiedErrorId

It seems 'FYI' abbreviation should be removed or expanded.
Please reformat and even a width of lines.

Updated.

Closed.

The comment as a single is obvious. So we should combining the comment with next one (move from line 194) to give a reader a full description.

Updated.

Closed.

We should put the file in 'asserts' subfolder.

moved it.

Closed.

The file has multiple wrong indentations. See raw view https://raw.githubusercontent.com/anmenaga/PowerShell/fd95dd19b90e7cb5d0094f52503b59c59ec1ca5b/test/powershell/Modules/Microsoft.PowerShell.Utility/Update-FormatData.Tests.ps1
It seems you copy-paste tabs.
Could you please reformat by first commit and put new code in next commits?

fixed indentations.

Closed.

Aditya Patwardhan (@adityapatwardhan) Can you update your review?

PR Changed since review

No blocking issues

Would testing $ps.Streams.Error give a more meaningful error if something went wrong in the test?

good idea; updated.

See previous

updated.

LGTM

…Shell#4139)

Update to ResourceManagerCache

fd95dd1

Microsoft Contribution License Agreements (msftclas) added the cla-not-required label Jun 29, 2017

Andrew (anmenaga) requested a review from Aditya Patwardhan (adityapatwardhan) June 29, 2017 00:55

Aditya Patwardhan (adityapatwardhan) previously requested changes Jun 29, 2017

View reviewed changes

Dongbo Wang (daxian-dbw) assigned Travis Plunk (TravisEz13) Jul 5, 2017

Andrew added 2 commits July 11, 2017 12:00

PR feedback

b1bc8f9

Updated format ps1xml

baddeeb

Andrew (anmenaga) mentioned this pull request Jul 14, 2017

format.ps1xml files referencing built-in assemblies fail to load #4256

Closed

Ilya (iSazonov) reviewed Jul 15, 2017

View reviewed changes

Andrew added 2 commits July 18, 2017 13:22

fixed tabs in indentation

4ed51b0

PR feedback

5bc3344

Travis Plunk (TravisEz13) reviewed Jul 21, 2017

View reviewed changes

minor update to verification in tests

70d5bbd

Travis Plunk (TravisEz13) approved these changes Jul 25, 2017

View reviewed changes

Aditya Patwardhan (adityapatwardhan) approved these changes Aug 4, 2017

View reviewed changes

Travis Plunk (TravisEz13) changed the title Making ResourceManagerCache check for alternative resource paths Make ResourceManagerCache check for alternative resource paths Aug 4, 2017

Travis Plunk (TravisEz13) merged commit 99236b1 into PowerShell:master Aug 4, 2017

Andrew (anmenaga) deleted the ResourceManagerCachePatch branch October 31, 2018 21:20

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj/PowerShell that referenced this pull request Aug 6, 2026

Make ResourceManagerCache check for alternative resource paths (Power…

cb41ae0

…Shell#4139)

+                          </TableControl>
+                      </View>
+                  </ViewDefinitions>
+              </Configuration>

                
                    No newline at end of file

+                          $ps.Invoke()
+              			$sma = [appdomain]::CurrentDomain.GetAssemblies() | ? { if ($_.Location) {$_.Location.EndsWith("System.Management.Automation.dll")}}
+              			$smaLocation = $sma.Location
+              			$ps.Streams.Error | %{ $_.Exception.Message.Contains($smaLocation) | Should be $true }

+                          $null = $ps.AddScript("Update-FormatData -PrependPath $formatFilePath")
+                          $ps.Streams.Error.Clear()
+                          $ps.Invoke()
+                          $ps.HadErrors | Should be $false

+                          $null = $ps.AddScript("Update-FormatData -PrependPath $formatFilePath")
+                          $ps.Streams.Error.Clear()
+                          $ps.Invoke()
+                          $ps.HadErrors | Should be $false

+                              }
+                              else
+                              {
+                                  newBaseName = assembly.GetName().Name + resourcesSubstring + baseName; // e.g. "System.Management.Automation.resources.FileSystemProviderStrings"

+                          // FYI: for a non-existing resource defined by {assembly,baseName,resourceId}
+                          // MissingManifestResourceException is thrown only at the time when resource retrieval method such as ResourceManager.GetString or ResourceManager.GetObject is called,
+                          // Not when you instantiate a ResourceManager object.



		Describe "Update-FormatData with resources in CustomControls" -Tags "CI" {

		@@ -0,0 +1,88 @@
		<Configuration>
		<SelectionSets>

Make ResourceManagerCache check for alternative resource paths by anmenaga · Pull Request #4139 · PowerShell/PowerShell · GitHub

Uh oh!

Make ResourceManagerCache check for alternative resource paths - #4139

Make ResourceManagerCache check for alternative resource paths#4139Travis Plunk (TravisEz13) merged 6 commits intoPowerShell:masterPowerShell/PowerShell:masterfrom anmenaga:ResourceManagerCachePatchanmenaga/PowerShell:ResourceManagerCachePatchCopy head branch name to clipboard

Conversation

Andrew (anmenaga) commented Jun 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) Jul 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Travis Plunk (TravisEz13) commented Jul 20, 2017

Uh oh!

Travis Plunk (TravisEz13) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Aditya Patwardhan (adityapatwardhan) left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Make ResourceManagerCache check for alternative resource paths#4139
Travis Plunk (TravisEz13) merged 6 commits into
PowerShell:masterfrom
anmenaga:ResourceManagerCachePatch

Ilya (iSazonov) Jul 15, 2017 •
edited
Loading