Skip to content
This repository was archived by the owner on Jun 30, 2023. It is now read-only.

Conversation

@MarkZuber
Copy link
Contributor

Please pay special attention to changes made in LoggerCallbackTests.cs and AdalLoggerBase::Log() to ensure behavior is consistent with legacy.

@bgavrilMS
Copy link
Member

bgavrilMS commented Oct 2, 2018

While having a look at the MSALLogger, I noticed that one of the methods throws an exception - is this expected? If not, probably missing a unit test as well.. Please check ADALLogger as well. Otherwise the PR looks good!

public override void InfoPii(string messageWithPii, string messageScrubbed)
{
throw new NotImplementedException();
}

{
var serviceEx = new AdalServiceException(AdalErrorMessage.UnauthorizedHttpStatusCodeExpected, ex);
string noPiiMsg = CoreExceptionFactory.Instance.GetPiiScrubbedDetails(serviceEx);
CoreLoggerBase.Default.Error(noPiiMsg);
Copy link
Member

@bgavrilMS bgavrilMS Oct 2, 2018

Choose a reason for hiding this comment

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

noPiiMsg [](start = 49, length = 8)

Tedious work, thanks for doing this! #ByDesign

Copy link
Member

@bgavrilMS bgavrilMS left a comment

Choose a reason for hiding this comment

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

:shipit:

@MarkZuber
Copy link
Contributor Author

      While having a look at the MSALLogger, I noticed that one of the methods throws an exception - is this expected? If not, probably missing a unit test as well.. Please check ADALLogger as well. Otherwise the PR looks good!

public override void InfoPii(string messageWithPii, string messageScrubbed)
{
throw new NotImplementedException();
}
You may have been looking at an intermediary commit. I've re-reviewed the code and I don't see any NotImplementedExceptions being thrown within any of the loggers.

@MarkZuber MarkZuber merged commit 0c6d6fd into dev Oct 2, 2018
@MarkZuber MarkZuber deleted the markzuber/logcleanup branch October 2, 2018 18:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants