Make AutoCompleteResourceV3 use the supplied ILogger (if one is supplied) #4287

tintoy · 2021-09-29T02:26:59Z

Otherwise fall back to NullLogger.Instance.

Bug

Description

The supplied logger is now used (and, if null, then NullLogger.Instance is used; which appears to be the previous behaviour).
I'm not sure what the original intent was, but it looks to me like the logger can't be null because at least one of the downstream components created / called by the methods in question will actually throw an ArgumentNullException if passed a null logger.

PR Checklist

PR has a meaningful title
PR has a linked issue.
Described changes
Tests
- Automated tests added
  There don't appear to be any existing tests that check if supplied loggers are used, but I'm happy to write one if you feel it is necessary.
- OR
- Test exception
- OR
- N/A
Documentation
- Documentation PR or issue filled
- OR
- N/A

erdembayar · 2021-09-29T17:25:31Z

src/NuGet.Core/NuGet.Protocol/Resources/AutoCompleteResourceV3.cs

@@ -51,8 +51,8 @@ public AutoCompleteResourceV3(HttpSource client, ServiceIndexResourceV3 serviceI

            var queryUri = queryUrl.Uri;
            var results = await _client.GetJObjectAsync(
-                new HttpSourceRequest(queryUri, Common.NullLogger.Instance),


Thank you for your contribution.
Maybe ILogger logger = log ?? Common.NullLogger.Instance; then reusing it might look better. Also adding asserting logger log in following places would be great idea.
https://github.com/NuGet/NuGet.Client/blob/dev/test/NuGet.Core.Tests/NuGet.Protocol.Tests/AutoCompleteResourceV3Tests.cs
https://github.com/NuGet/NuGet.Client/blob/dev/test/NuGet.Core.Tests/NuGet.Protocol.Tests/PowershellAutoCompleteResourceTests.cs

Thanks, will do!

Please ping me once you make push changes.

Sorry, @erdembayar - quick question:

Also adding asserting logger log in following places would be great idea.

If I understand this correctly, the test should assert that the supplied ILogger instance is used, and not NullLogger.Instance?

NullLogger is a singleton (and there doesn't seem to be any other test-friendly implementation of ILogger that I can easily create an instance of, in order to verify that the supplied ILogger instance is used rather than NullLogger.Instance).

Off the top of my head, options for this are:

Use Moq's Mock<T> to mock out an ILogger (potentially tricky because I don't know what calls downstream components will make to it so it might make the test a little fragile)

Create a new logger class (derived from LoggerBase) just for use in these tests

Just try to get a separate instance of NullLogger to play with (eg. via Activator.CreateInstance or similar)?

I would go with 2nd option. You can create new logger class implements ILogger and pass to resource and check if anything is logged into logger after operation.
If you see anything in logged then it's good.

You can create new logger class implements ILogger

We have a TestLogger.

Please uste that.

Ok, I've added a logging check to the test for IdStartsWithAsync, but there isn't a test for VersionStartsWithAsync. If you're happy with the changes I made to the first test I'm happy to have a go at writing the second one.

Please check my comment.

erdembayar · 2021-10-01T21:44:44Z

test/NuGet.Core.Tests/NuGet.Protocol.Tests/AutoCompleteResourceV3Tests.cs


            // Assert
            Assert.Equal(10, result.Result.Count());
+            Assert.NotEqual(0, logger.Messages.Count);


It's going correct direction. But we need some action which logs something into logger.Messages. If it's 0 then it's hard to tell if we're not logging correctly or nothing really happened.
Please try to find scenario that logs something. Currently nothing comes into my mind that logs something here.

@tintoy - Sorry for the delay in responding on this pull request. Looking at the code path I could not find any scenario where errors are logged to the logger. I noticed only information messages added to the logger. I am fine with the current test unless other team members have unique perspective.

Can you please modify tests in the below class file because AutoCompleteResource is also used in powershell commands?

For e.g.

NuGet.Client/test/NuGet.Core.Tests/NuGet.Protocol.Tests/PowershellAutoCompleteResourceTests.cs

Line 98 in 3d301ae

public async Task PowershellAutoComplete_IdStartsWithCancelsAsAppropriate(string sourceUrl)

Please rebase your branch with dev to get latest.

No problem, sorry this one kinda dropped off my radar for a while too. Will get onto it ASAP :)

So far, I don't see any test which actually logging anything.
We need something not 0 and content of message is what you intended to log.

Ok, no worries; can I assume that any of the messages currently logged (based on inspection of the code) can be considered a suitable candidate?

For what it's worth, the reason I wanted to fix this bug wasn't because of any logging done by AutoCompleteResourceV3, but because of messages logged by a downstream component (HttpSourceRequest) that AutoCompleteResourceV3 was passing NullLogger.Instance to rather than the supplied logger:

9c9846d#diff-ffa236dbba0f0c9de9e65a973bbfbee253081710ee2bf20eeab2f3a9ef51d6b8L54

I guess what I'm getting at is that the this method doesn't log anything itself, but a downstream component that it uses does, and perhaps this is what we can test for?

I don't know any message can be logged here, but if we're adding logging then better to something logged in there to test. What is your use case? I assume you already have one, then just add test for it.

Sorry, I misread this, LGTM.

Otherwise fall back to NullLogger.Instance. NuGet/Home#11272

NuGet/Home#11272

…V3.IdStartsWith NuGet/Home#11272

tintoy · 2022-02-05T02:04:18Z

Hmm, I think perhaps this wasn't the rebase I was after.

zivkan · 2022-02-05T16:35:08Z

You did a merge (commit 2a3ac2c), rather than a rebase + force push. Can you please try to fix it, as GitHub is showing all the files modified by everyone else's PRs, making it really difficult to figure out what your PR is actually changing.

tintoy · 2022-02-06T00:39:43Z

Oops - thanks, that's what I meant to do!

…gger is used when calling AutoCompleteResource NuGet/Home#11272

kartheekp-ms

👏👏👏👏

tintoy requested a review from a team as a code owner September 29, 2021 02:27

ghost added the Community PRs created by someone not in the NuGet team label Sep 29, 2021

erdembayar added the Approved for CI label Sep 29, 2021

erdembayar reviewed Sep 29, 2021

View reviewed changes

erdembayar removed the Approved for CI label Oct 1, 2021

erdembayar reviewed Oct 1, 2021

View reviewed changes

erdembayar added the Approved for CI label Oct 1, 2021

zivkan assigned kartheekp-ms Jan 25, 2022

Adam Friedman added 3 commits February 5, 2022 12:35

Make AutoCompleteResourceV3 use the supplied logger (if one is supplied)

9c9846d

Otherwise fall back to NullLogger.Instance. NuGet/Home#11272

Changes to AutoCompleteResource based on pull-request feedback

6c3ba8e

NuGet/Home#11272

Verify that supplied logger is used when calling AutoCompleteResource…

b11736f

…V3.IdStartsWith NuGet/Home#11272

Update PowershellAutoCompleteResourceTests to verify that supplied lo…

2f1e661

…gger is used when calling AutoCompleteResource NuGet/Home#11272

tintoy force-pushed the dev-tintoy-client-loggers branch from 1e7a40b to 2f1e661 Compare February 6, 2022 00:44

kartheekp-ms added Approved for CI and removed Approved for CI labels Feb 10, 2022

kartheekp-ms approved these changes Feb 11, 2022

View reviewed changes

kartheekp-ms requested review from erdembayar and nkolev92 February 12, 2022 00:53

erdembayar approved these changes Feb 15, 2022

View reviewed changes

kartheekp-ms requested a review from erdembayar February 15, 2022 18:32

kartheekp-ms merged commit 82e9aa3 into NuGet:dev Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make AutoCompleteResourceV3 use the supplied ILogger (if one is supplied) #4287

Make AutoCompleteResourceV3 use the supplied ILogger (if one is supplied) #4287

tintoy commented Sep 29, 2021

erdembayar Sep 29, 2021

tintoy Sep 29, 2021

erdembayar Sep 29, 2021

tintoy Sep 30, 2021 •

edited

erdembayar Sep 30, 2021

nkolev92 Sep 30, 2021

tintoy Oct 1, 2021

erdembayar Oct 1, 2021

erdembayar Oct 1, 2021

kartheekp-ms Feb 2, 2022

tintoy Feb 2, 2022

erdembayar Feb 11, 2022

tintoy Feb 11, 2022

tintoy Feb 11, 2022

erdembayar Feb 14, 2022 •

edited

tintoy commented Feb 5, 2022

zivkan commented Feb 5, 2022

tintoy commented Feb 6, 2022

kartheekp-ms left a comment

Make AutoCompleteResourceV3 use the supplied ILogger (if one is supplied) #4287

Make AutoCompleteResourceV3 use the supplied ILogger (if one is supplied) #4287

Conversation

tintoy commented Sep 29, 2021

Bug

Description

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tintoy Sep 30, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erdembayar Feb 14, 2022 • edited

Choose a reason for hiding this comment

tintoy commented Feb 5, 2022

zivkan commented Feb 5, 2022

tintoy commented Feb 6, 2022

kartheekp-ms left a comment

Choose a reason for hiding this comment

tintoy Sep 30, 2021 •

edited

erdembayar Feb 14, 2022 •

edited