Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sanitize localized stack trace in exception tests #9132

Merged
merged 1 commit into from May 2, 2021

Conversation

mstv
Copy link
Member

@mstv mstv commented Apr 28, 2021

Proposed changes

Proposed changes

  • Use fixed culture en-US in SerializableExceptionTests
  • Simplify regex

Screenshots

Before

image

After

image

Test methodology

  • adapt NUnit test

Test environment(s)

  • Git Extensions 33.33.33
  • Build a294965
  • Git 2.31.1.windows.1
  • Microsoft Windows NT 10.0.19042.0
  • .NET Framework 4.8.4341.0
  • DPI 96dpi (no scaling)

✒️ I contribute this code under The Developer Certificate of Origin.

@mstv mstv self-assigned this Apr 28, 2021
Copy link
Member

@gerhardol gerhardol left a comment

Choose a reason for hiding this comment

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

I would like to have this commented, explaining the three manipulations that are done.
It feels like these hardcoded assumptions has a risk of failing in the future (there may be languages that use two words for "at", right-to-left etc).

Draft?

Adjust the log for languages

  1. Replace Inner exception divider
  2. Replace the stack trace preposition
  3. Remove file path (to make compare results)

@mstv
Copy link
Member Author

mstv commented Apr 28, 2021

I have added the comments and noticed that the previous regex contained the preposition "in".

@RussKie
Copy link
Member

RussKie commented Apr 29, 2021 via email

@RussKie
Copy link
Member

RussKie commented Apr 30, 2021

Did you try applying the following to the fixture?

    [SetCulture("en-US")]
    [SetUICulture("en-US")]

@RussKie RussKie added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Apr 30, 2021
@ghost ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Apr 30, 2021
@mstv
Copy link
Member Author

mstv commented Apr 30, 2021

Did you try applying the following to the fixture?

    [SetCulture("en-US")]
    [SetUICulture("en-US")]

Thank you, it works.
I kept the - IMO simplified - regex.

@RussKie
Copy link
Member

RussKie commented Apr 30, 2021 via email

@mstv
Copy link
Member Author

mstv commented May 1, 2021

We set culture for locale sensitive tests. We ought to do the same here, and avoid custom logic.

I did so yesterday.
In addition, I simplified the existing replacement code.

@RussKie
Copy link
Member

RussKie commented May 1, 2021

I didn't quite grasp the response. Should've opened the diff.

Thank you

@mstv mstv merged commit a2cb7cf into gitextensions:master May 2, 2021
@mstv mstv deleted the fix/bugreporter_tests branch May 2, 2021 09:22
@ghost ghost added this to the 3.6 milestone May 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants