Skip to content

Conversation

@alexmg
Copy link
Contributor

@alexmg alexmg commented Oct 22, 2024

Motivation and Context (Why the change? What's the scenario?)

Contributing unit tests for the SensitiveDataLogger to cover a recently fixed bug and prevent future regression.

High level description (Approach, Design)

N/A - unit tests only

@alexmg alexmg requested a review from dluc as a code owner October 22, 2024 00:41
@alexmg
Copy link
Contributor Author

alexmg commented Oct 22, 2024

These unit tests provide coverage for the problem raised in #851

@alexmg
Copy link
Contributor Author

alexmg commented Oct 22, 2024

@microsoft-github-policy-service agree

Copy link
Collaborator

@dluc dluc left a comment

Choose a reason for hiding this comment

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

Minor changes for readability and to handle string case. Fixed Dispose.

One thing I would improve is making sure tests do not run in parallel, because they act on shared state, e.g. the static .Enabled and the env var. I tried adding an

Assert.False(SensitiveDataLogger.Enabled);

at the start of each test.

@dluc dluc merged commit ad3e241 into microsoft:main Oct 22, 2024
6 checks passed
@alexmg alexmg deleted the SensitiveDataLoggerTests branch October 22, 2024 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants