FixStacktraceLanguage #2391

Akturos · 2023-09-20T20:40:07Z

PR Summary

"Stacktrace is not filtered in non-english system languages" I found the PR #2062 and completed the approach to fix the issue.
The new function get's the correct StackTracke language informations and passes them to necessary the regex filters.

PR Checklist

PR has meaningful title
Summary describes changes
PR is ready to be merged
- If not, use the arrow next to Create Pull Request to mark it as a draft. PR can be marked Ready for review when it's ready.
Tests are added/update (if required)
Documentation is updated/added (if required)

fflaten · 2023-09-27T16:55:36Z

Thanks for looking into this. Will test and review properly later, but wanted to quickly mention this draft PR #2276. They differ in a couple ways:

You gather language at run start vs on every call
- Pro: Better performance 👍
You use resource (smart!) vs throwing a real exception
- Pro: Performance (?) and clean
- Con: Not compatible with PSv3 and 4 which we currently support. PS v3 and v4 are end-of-life so they're not critical, but atm. Pester supports them. So maybe throwing a real exception like the linked PR is worth it for now?

It would be nice to have a way to test this during our CI to avoid breaking it in the future. Ex. by accepting an optional [string]$StackTrace parameter so we could test the regex-pattern using 2-3 language samples.

Akturos · 2023-09-27T21:13:00Z

Hi @fflaten

I have adapted the code according to your wishes and added a fallback scenario for all versions to the regex of PR #2276. I also separated this function and added a parameter [string]$StackTrace which allows you to test this function. But after the commit there was a bug in PS6_2

https://github.com/pester/Pester/blob/7ca9c814cf32334303f7c506beaa6b1541554973/tst/Pester.RSpec.Demo.ts.ps1#L132C3-L132C68

which points to a runtime error. Can you please run the failed tests again, I think it's probably a onetime timing error.

The tests are successfully complete now.. so everything is OK

nohwnd

Thanks for the PR :)

As mentioned above, a test would be nice. I know there is Using-Culture either in this codebase or easily found on the internet, which should make this testable, and not dependent on the OS language.

Also a nitpick: local variables and keywords start with lowercase in this codebase.

nohwnd · 2023-10-14T11:27:45Z

src/Pester.Runtime.ps1

+        }
+        else {
+            #Prior to version 5 there are no ressources in the Assembly available (System.Management.Automation)
+            throw 'Fallback for PSVersion 3/4'


Using exceptions for control flow is not ideal, this will always throw for powershell 4, if you just reduce the scope of the try to be in the positive leg of the if it should make the code no more complicated, and avoid this issue.

I agree. Please move fallback exception to be part of Get-StackTraceLanguageFallBack. That way we can remove the large try/catch here and just call the fallback if no result or PS <5.

nohwnd · 2023-11-09T09:23:02Z

@Akturos will you address that PR remarks, or should I finish it up so we can ship it? :)

Akturos · 2023-11-10T00:14:11Z

@nohwnd

When modifying the code, I noticed that without throwing a exception for PS version 3/4, the necessary values for the stacktracelanguage can no longer be read from the self-generated error. This raises the question of whether you really want it that way, as the error can also be used to generate the appropriate stacktracelanguage for PS version 3/4. If the error is removed, this will no longer work and only English will be the default for PS version 3 / 4.

I have adapted the variables to the default notation.

Regarding the test. I have tried to build a test with the cultures. However, this is not possible, or I have not managed to set the cultures in the PS session in such a way that I could use them for a test.

& { $stackTraceRessourceName = 'DebuggerStrings' [System.Threading.Thread]::CurrentThread.CurrentCulture = 'fr-FR' [System.Threading.Thread]::CurrentThread.CurrentUICulture = 'fr-FR' $r = [System.Resources.ResourceManager]::new($stackTraceRessourceName, [System.Reflection.Assembly]::GetAssembly([System.Management.Automation.Breakpoint])) $r.GetString('StackTraceFormat') }
Maybe you have a better idea.

Cheers
Acturos

nohwnd · 2023-11-14T10:00:37Z

When modifying the code, I noticed that without throwing a exception for PS version 3/4, the necessary values for the stacktracelanguage can no longer be read from the self-generated error.

We can probably throw once and get the elements from that? And then use normal control flow? Extensions are not super expensive, on my system it is 30ms to throw and catch 1000 of them, still it is IMHO unnecessary to throw and catch on every assertion fail.

fflaten · 2023-11-14T11:41:48Z

We can probably throw once and get the elements from that?

Isn't it executed once during import and cached in a module variable?

Personal opinion:

Change to once per run in case it's possible to adjust culture in current process?
I'd refactor control flow to return early instead of nested if/else try/catch.

claudiospizzi · 2023-11-18T14:03:19Z

Hi all

I agree with @fflaten - the Idea was to store the stack trace language once in module scoped variable $Script:stackTraceLanguage. This is done in the script Pester.Runtime.ps1, which is loaded once during module import, right?

Throwing an exception once and only for rare cases (PSVersion < 5 or if there is any unexpected issue with the resource manager) sounds acceptable.

Would be cool, if we can merge that to have a better experience for non-English systems.

Thank you.

…os/Pester into FixStackTraceLanguageIssue

Akturos · 2023-11-23T21:57:10Z

Hi all
I also agree with @fflaten. I have reset the branch to the commit ce24250. I think this is the most useful solution. I have also adjusted the variables.

It would be great if we could ship this to prdouction

Akturos · 2024-01-07T00:19:33Z

Happy new year all.

Is there an update from your side guys? Should anything else be adjusted or can we ship it. Thanks for your feedback.

fflaten · 2024-01-11T20:36:06Z

src/Pester.Runtime.ps1

+    }
+    $Regex = "(?<At>.*)\s(?<ScriptBlockOrFunction>\<\w+\>),\s(?<FileName>\<.+\>)\s*:\s(?<Line>\w+)\s(?<LineNumber>\w+)\z"


Suggested change

}

$Regex = "(?<At>.*)\s(?<ScriptBlockOrFunction>\<\w+\>),\s(?<FileName>\<.+\>)\s*:\s(?<Line>\w+)\s(?<LineNumber>\w+)\z"

}

$Regex = "(?<At>.*)\s(?<ScriptBlock>\<\w+\>),\s(?<NoFile>\<.+\>)\s*:\s(?<Line>.+)\s(?<LineNumber>\d+)\z"

Not sure if "line" could be multiple words, but regex should be consistent between this and Get-StackTraceLanguage.

Also consider comment to make it clear that this only matches final line in stacktrace with scriptblock-source, e.g. at <ScriptBlock>, <No file>: line 1

fflaten · 2024-01-11T20:42:37Z

src/Pester.Runtime.ps1

+function Get-StackTraceLanguage {
+    #Full fallback scenario to the solution of PR #2276 in case of error
+    try {
+        if ($PSVersionTable.PSVersion.Major -ge 5) {


Consider reversing this. Easier to read and less nesting.
See related comment #2391 (comment)

# Required string resource missing prior to version 5 - use fallback if ($PSVersionTable.PSVersion.Major -lt 5) { return (Get-StackTraceLanguageFallBack) } if ($PSVersionTable.PSVersion.Major -gt 5) { $StackTraceRessourceName = 'System.Management.Automation.resources.DebuggerStrings' .... # rest of PS 5+ code.

fflaten · 2024-01-11T20:47:43Z

Happy new year! 🙂 I've added a review with a few change requests.

I think nohwnd's a bit busy, so final approval + merge might take a little time.

nohwnd · 2024-04-06T18:50:32Z

I am here now and will be for few more weeks at least, so best time to resume work on this :))

nohwnd · 2024-04-10T23:09:56Z

src/Pester.Runtime.ps1

+        }
+    }
+}
+#defining script variable


Suggested change

#defining script variable

nohwnd · 2024-04-10T23:11:17Z

src/Pester.Runtime.ps1

+function Get-StackTraceLanguage {
+    $err = try { throw "err" }  catch { $_ }
+    $firstFrame = ($err.ScriptStackTrace -split [System.Environment]::NewLine, 2 )[0]
+    if ($firstFrame -match "(?<at>.[^G]?)\s+Get-StackTraceLanguage(?<separator>.).+:\s+(?<line>.\S+)\s\d+") {
+        @{
+            At        = $Matches["at"]
+            Separator = $Matches["separator"]
+            Line      = $Matches["line"]
+        }
+    }
+}


Changed your code to parse it from exception that we throw, as originally suggested. This should be cheaper and easier to replicate than relying on internal resource names because we rely on public api.

nohwnd · 2024-04-10T23:13:53Z

Please figure out a way to test this. Changing current thread culture did not affect stack trace for me. And I don't think the proposed code was working because the regex was rewritten few lines below by the if ($true) code. So I would like a way to ensure this works and keeps working.

[System.Threading.Thread]::CurrentThread.CurrentCulture = [System.Globalization.CultureInfo]::GetCultureInfo("cs-cz")
[System.Threading.Thread]::CurrentThread.CurrentUiCulture = [System.Globalization.CultureInfo]::GetCultureInfo("cs-cz")

1.5
function a {
    try { throw "a" } catch { $_.ScriptStackTrace }
}

nohwnd · 2024-04-25T06:22:07Z

I know what I broke in the code, the # PESTER BUILD is a build directive that emits that code only when we are building locally without inlining all the scripts into single pester.psm1 file. I forgot about that, and thought the code always runs because of it if($true). It is probably better to use that pattern of removing all in the folder, because there is also pester.ps1 that is not filtered now.

Sandro Barbera added 2 commits September 20, 2023 22:29

FixStacktraceLanguage

0bdea48

Fix exclude for StackTraceLanguage for POSH V3/4

b2410c4

Akturos marked this pull request as ready for review September 20, 2023 22:22

Enhanced Fallback for all Version and Support for POSH 3/4

ce24250

nohwnd reviewed Oct 14, 2023

View reviewed changes

Version 4 Error optimizing

59c9c93

Sandro Barbera added 4 commits November 23, 2023 21:54

Reset to 27.09

086fddf

Merge branch 'FixStackTraceLanguageIssue' of https://github.com/Aktur…

19ca5e9

…os/Pester into FixStackTraceLanguageIssue

Variabels corrected

4d0a432

Revert to Commit from 27.09

8c25c0a

fflaten requested changes Jan 11, 2024

View reviewed changes

nohwnd added 2 commits April 10, 2024 23:52

Merge branch 'main' into FixStackTraceLanguageIssue

f3d27b1

Parse words from exception

f352060

nohwnd reviewed Apr 10, 2024

View reviewed changes

src/Pester.Runtime.ps1

}

}

}

#defining script variable

Copy link

Member

nohwnd Apr 10, 2024

Choose a reason for hiding this comment

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

Suggested change

#defining script variable

nohwnd reviewed Apr 10, 2024

View reviewed changes

nohwnd mentioned this pull request Apr 12, 2024

Fix issue pester#2062 "Stacktrace is not filtered in non-english system languages" #2276

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FixStacktraceLanguage #2391

FixStacktraceLanguage #2391

Akturos commented Sep 20, 2023

fflaten commented Sep 27, 2023 •

edited

Akturos commented Sep 27, 2023 •

edited

nohwnd left a comment

nohwnd Oct 14, 2023

fflaten Jan 11, 2024 •

edited

nohwnd commented Nov 9, 2023

Akturos commented Nov 10, 2023

nohwnd commented Nov 14, 2023

fflaten commented Nov 14, 2023 •

edited

claudiospizzi commented Nov 18, 2023

Akturos commented Nov 23, 2023

Akturos commented Jan 7, 2024

fflaten Jan 11, 2024 •

edited

fflaten Jan 11, 2024 •

edited

fflaten commented Jan 11, 2024 •

edited

nohwnd commented Apr 6, 2024

nohwnd Apr 10, 2024

nohwnd Apr 10, 2024

nohwnd commented Apr 10, 2024

nohwnd commented Apr 25, 2024

		}
		$Regex = "(?<At>.)\s(?<ScriptBlockOrFunction>\<\w+\>),\s(?<FileName>\<.+\>)\s:\s(?<Line>\w+)\s(?<LineNumber>\w+)\z"

FixStacktraceLanguage #2391

Are you sure you want to change the base?

FixStacktraceLanguage #2391

Conversation

Akturos commented Sep 20, 2023

PR Summary

PR Checklist

fflaten commented Sep 27, 2023 • edited

Akturos commented Sep 27, 2023 • edited

nohwnd left a comment

Choose a reason for hiding this comment

nohwnd Oct 14, 2023

Choose a reason for hiding this comment

fflaten Jan 11, 2024 • edited

Choose a reason for hiding this comment

nohwnd commented Nov 9, 2023

Akturos commented Nov 10, 2023

nohwnd commented Nov 14, 2023

fflaten commented Nov 14, 2023 • edited

claudiospizzi commented Nov 18, 2023

Akturos commented Nov 23, 2023

Akturos commented Jan 7, 2024

fflaten Jan 11, 2024 • edited

Choose a reason for hiding this comment

fflaten Jan 11, 2024 • edited

Choose a reason for hiding this comment

fflaten commented Jan 11, 2024 • edited

nohwnd commented Apr 6, 2024

nohwnd Apr 10, 2024

Choose a reason for hiding this comment

nohwnd Apr 10, 2024

Choose a reason for hiding this comment

nohwnd commented Apr 10, 2024

nohwnd commented Apr 25, 2024

fflaten commented Sep 27, 2023 •

edited

Akturos commented Sep 27, 2023 •

edited

fflaten Jan 11, 2024 •

edited

fflaten commented Nov 14, 2023 •

edited

fflaten Jan 11, 2024 •

edited

fflaten Jan 11, 2024 •

edited

fflaten commented Jan 11, 2024 •

edited