Conversation
Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThreads an ILogger through OrcaHelloFetcher public/private methods, replaces Console.Error calls with logger.LogError, updates call sites (page models, tests) to pass loggers, and converts many Fetcher logs to structured logging. Signatures and XML docs updated; behavior otherwise unchanged. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
OrcanodeMonitor/Core/OrcaHelloFetcher.cs (1)
153-157: Inconsistent exception logging — passexas the first parameter for structured logging.Several
LogErrorcalls embed the exception in the message string (e.g.,$"...{ex}") instead of passing it as the dedicatedExceptionparameter. This prevents structured log sinks (Application Insights, Seq, etc.) from capturing the exception object properly. Lines 353, 450, 770, 821, 898, and 944 have the same issue, while lines 573 and 720 correctly uselogger.LogError(ex, "…").Example fix for this call site (apply the same pattern elsewhere)
- logger.LogError($"[GetOrcaHelloNodeAsync] Error retrieving node info for '{nodeName}': {ex}"); + logger.LogError(ex, "[GetOrcaHelloNodeAsync] Error retrieving node info for '{NodeName}'", nodeName);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@OrcanodeMonitor/Core/OrcaHelloFetcher.cs` around lines 153 - 157, In the GetOrcaHelloNodeAsync catch block, stop embedding the exception into the formatted message and instead call logger.LogError(ex, "...") so the Exception is passed as the dedicated first parameter (e.g., logger.LogError(ex, "[GetOrcaHelloNodeAsync] Error retrieving node info for '{nodeName}'");); update the LogError call using the logger instance and the nodeName placeholder accordingly, and apply the same fix to the other LogError call sites that currently append the exception into the message so all use logger.LogError(exception, "descriptive message with placeholders") for proper structured logging.Test/OrcaHelloFetcherTests.cs (2)
20-28: Logger returned from a disposed factory will silently drop all log output.
using var loggerFactorycausesDispose()to be called on the factory before the method returns.ConsoleLoggerProviderhalts its background processing queue on disposal, so everylogger.LogError()/logger.LogXxx()call made against the returnedILoggeris a silent no-op. Tests still pass because nothing asserts on log output, but log lines are invisible when debugging test failures.The same pattern appears in
Test/FetcherTests.cslines 25-33 and is effectively duplicated here.♻️ Proposed fix — keep the factory alive for the test's lifetime
- private ILogger CreateConsoleLogger() - { - using var loggerFactory = LoggerFactory.Create(builder => - { - builder.AddConsole(); - builder.SetMinimumLevel(LogLevel.Debug); - }); - return loggerFactory.CreateLogger<OrcaHelloFetcherTests>(); - } + private ILoggerFactory _loggerFactory; + + private ILogger CreateConsoleLogger() + { + _loggerFactory = LoggerFactory.Create(builder => + { + builder.AddConsole(); + builder.SetMinimumLevel(LogLevel.Debug); + }); + return _loggerFactory.CreateLogger<OrcaHelloFetcherTests>(); + }And clean up in a
[TestCleanup]method:[TestCleanup] public void TestsCleanup() { _loggerFactory?.Dispose(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Test/OrcaHelloFetcherTests.cs` around lines 20 - 28, The CreateConsoleLogger method currently disposes the LoggerFactory before returning an ILogger (using `using var loggerFactory`), causing the returned logger to drop all output; change the implementation to store a non-disposed LoggerFactory instance (e.g., a private field like `_loggerFactory`) instead of using `using`, call `LoggerFactory.Create` into that field, and return `_loggerFactory.CreateLogger<OrcaHelloFetcherTests>()`; mirror this change in the duplicated implementation in `FetcherTests` and add a test teardown method (e.g., a `[TestCleanup]` method `TestsCleanup` that calls `_loggerFactory?.Dispose()`) to dispose the factory at test suite cleanup.
18-18:_loggerfield should beprivate.The field is implicitly
privatein C#, but the explicit modifier is the idiomatic convention for test-class fields and improves clarity.✏️ Proposed fix
- ILogger _logger; + private ILogger _logger;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Test/OrcaHelloFetcherTests.cs` at line 18, The _logger field in the OrcaHelloFetcherTests class is declared without an access modifier; change the declaration for the field named _logger to explicitly include the private modifier (e.g., private ILogger _logger) to follow C# idiomatic style and improve clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@OrcanodeMonitor/Pages/OrcaHelloNode.cshtml.cs`:
- Line 15: The page model is requesting the non-generic ILogger which isn't
registered in DI; change the private field and constructor injection to use
ILogger<OrcaHelloNodeModel> instead of ILogger, i.e., update the type of the
_logger field and the constructor parameter in the OrcaHelloNodeModel class to
ILogger<OrcaHelloNodeModel> and assign it as before; also review/replace any
other occurrences in this file (notably the constructor/fields around the other
mentions at lines ~53-56) so all logger uses are the typed
ILogger<OrcaHelloNodeModel>.
In `@Test/FetcherTests.cs`:
- Around line 26-34: CreateConsoleLogger currently disposes LoggerFactory before
the returned ILogger can use its providers, causing logs to be dropped; change
loggerFactory from a local using variable to a class-level field (e.g.,
_loggerFactory), initialize it in CreateConsoleLogger without disposing, return
loggerFactory.CreateLogger<FetcherTests>() (use the correct category type), and
add a TestCleanup/Dispose method (e.g., TearDown or DisposeTestResources) that
calls _loggerFactory?.Dispose() to clean up after tests.
---
Nitpick comments:
In `@OrcanodeMonitor/Core/OrcaHelloFetcher.cs`:
- Around line 153-157: In the GetOrcaHelloNodeAsync catch block, stop embedding
the exception into the formatted message and instead call logger.LogError(ex,
"...") so the Exception is passed as the dedicated first parameter (e.g.,
logger.LogError(ex, "[GetOrcaHelloNodeAsync] Error retrieving node info for
'{nodeName}'");); update the LogError call using the logger instance and the
nodeName placeholder accordingly, and apply the same fix to the other LogError
call sites that currently append the exception into the message so all use
logger.LogError(exception, "descriptive message with placeholders") for proper
structured logging.
In `@Test/OrcaHelloFetcherTests.cs`:
- Around line 20-28: The CreateConsoleLogger method currently disposes the
LoggerFactory before returning an ILogger (using `using var loggerFactory`),
causing the returned logger to drop all output; change the implementation to
store a non-disposed LoggerFactory instance (e.g., a private field like
`_loggerFactory`) instead of using `using`, call `LoggerFactory.Create` into
that field, and return `_loggerFactory.CreateLogger<OrcaHelloFetcherTests>()`;
mirror this change in the duplicated implementation in `FetcherTests` and add a
test teardown method (e.g., a `[TestCleanup]` method `TestsCleanup` that calls
`_loggerFactory?.Dispose()`) to dispose the factory at test suite cleanup.
- Line 18: The _logger field in the OrcaHelloFetcherTests class is declared
without an access modifier; change the declaration for the field named _logger
to explicitly include the private modifier (e.g., private ILogger _logger) to
follow C# idiomatic style and improve clarity.
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request refactors the OrcaHelloFetcher class and related code to use ASP.NET Core's structured logging (ILogger) instead of direct Console.WriteLine and Console.Error.WriteLine calls. This improves logging consistency and enables better log management in production environments.
Changes:
- Replaced all
Console.Error.WriteLinecalls inOrcaHelloFetcherwithlogger.LogErrorcalls - Added
ILoggerparameters to multiple methods inOrcaHelloFetcherthat previously wrote to console - Updated all call sites across Razor Pages and tests to pass logger instances to the modified methods
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| OrcanodeMonitor/Core/OrcaHelloFetcher.cs | Replaced Console.Error.WriteLine with logger.LogError; added ILogger parameters to 9 methods |
| OrcanodeMonitor/Pages/OrcaHelloPod.cshtml.cs | Updated method calls to pass _logger parameter |
| OrcanodeMonitor/Pages/OrcaHelloOverview.cshtml.cs | Updated method calls to pass _logger parameter |
| OrcanodeMonitor/Pages/OrcaHelloNode.cshtml.cs | Added ILogger field and constructor parameter; updated method calls to pass _logger |
| OrcanodeMonitor/Pages/Index.cshtml.cs | Updated method call to pass _logger parameter |
| OrcanodeMonitor/Pages/Detections.cshtml.cs | Updated method calls to pass _logger parameter |
| Test/OrcaHelloFetcherTests.cs | Added logger setup with CreateConsoleLogger helper method; updated test method calls |
| Test/FetcherTests.cs | Replaced Mock with CreateConsoleLogger helper method; updated test method call |
| OrcanodeMonitor/Program.cs | Minor comment punctuation fix |
Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
OrcanodeMonitor/Core/OrcaHelloFetcher.cs (1)
464-478:⚠️ Potential issue | 🟡 MinorSpurious
LogErrorfor conditions that are expected in offline/test mode.
GetOrcaHelloPodAsyncis the only method in this file that logs an error when_k8sClientis null (Line 467) or when no best pod is found (Line 476). Every other method with an identical null-client guard —GetOrcaHelloNodeAsync,GetPodCommandOutput,FetchPodMetricsAsync,FetchNodeMetricsAsync,UpdateOrcaHelloDataAsync, andGetOrcaHelloLogAsync— returns silently. A null client is the expected state in offline mode and tests, so this will generate false-alarm errors in those environments (confirmed byOrcaHelloFetcherTests.GetOrcaHelloPodAsync_ReturnsNull_WhenClientIsNullwhich exercises this exact path). Similarly, a node having no matching pod is a normal operational condition.🔧 Proposed fix (align with the rest of the file)
- if (client == null) - { - logger.LogError($"[GetOrcaHelloPodAsync] Kubernetes client is null"); - return null; - } + if (client == null) + { + return null; + } string namespaceName = orcanode.OrcasoundSlug; V1PodList pods = await client.CoreV1.ListNamespacedPodAsync(namespaceName); GetBestPodStatus(pods.Items, out V1Pod? bestPod, out V1ContainerStatus? bestContainerStatus); if (bestPod == null) { - logger.LogError($"[GetOrcaHelloPodAsync] Best pod is null"); - return null; + return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@OrcanodeMonitor/Core/OrcaHelloFetcher.cs` around lines 464 - 478, GetOrcaHelloPodAsync currently logs errors when _k8sClient is null and when GetBestPodStatus yields no bestPod, which is inconsistent with other methods; change the early-exit branches in GetOrcaHelloPodAsync so they mirror the others by removing the logger.LogError calls (or demoting them to Debug/Trace) and simply return null when _k8sClient is null or bestPod is null after calling GetBestPodStatus, keeping the rest of the method unchanged.
🧹 Nitpick comments (2)
Test/OrcaHelloFetcherTests.cs (1)
18-26:CreateConsoleLoggeris duplicated fromFetcherTests.This method is functionally identical to
FetcherTests.CreateConsoleLogger(lines 24–32 of that file), differing only in the generic type argument. Consider extracting a shared static test helper — e.g.,TestLoggerHelper.CreateConsoleLogger<T>()— to avoid maintaining two copies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Test/OrcaHelloFetcherTests.cs` around lines 18 - 26, The CreateConsoleLogger method in OrcaHelloFetcherTests duplicates FetcherTests.CreateConsoleLogger; extract a shared static helper (e.g., TestLoggerHelper.CreateConsoleLogger<T>()) that builds a LoggerFactory with builder.AddConsole() and SetMinimumLevel(LogLevel.Debug) and returns CreateLogger<T>(), then replace both FetcherTests.CreateConsoleLogger and OrcaHelloFetcherTests.CreateConsoleLogger to call TestLoggerHelper.CreateConsoleLogger<T>() (use the test class type as T) and remove the duplicated methods.OrcanodeMonitor/Core/OrcaHelloFetcher.cs (1)
87-101: Use message templates instead of string interpolation inLogErrorcalls (CA2254).All introduced
logger.LogErrorcalls use$"..."string interpolation. A message template passed to a logger API should not be a non-constant string; using string interpolation means the template varies between calls. This has two practical downsides:
- String interpolation in logging is problematic for performance, as the string is evaluated even if the corresponding
LogLevelisn't enabled — the interpolated value is evaluated at the point of passing the string, not after the log message is ready to be sent to a provider.- Structured logging providers expect named placeholder arguments to be passed separately, as the arguments themselves are passed to the logging system, not just the formatted message template, enabling providers to store the parameter values as individual queryable fields.
Several of the new calls also use
$"..."on purely static strings with no interpolated variable at all (Lines 87, 95, 101, 476, 573, 720, 770, 821, 898, 944) — drop the$prefix there. For calls with actual variables, use message templates:♻️ Representative fixes
-logger.LogError($"[CreateK8sClient] No KUBERNETES_CA_CERT"); +logger.LogError("[CreateK8sClient] No KUBERNETES_CA_CERT"); -logger.LogError(ex, $"[GetOrcaHelloNodeAsync] Error retrieving node info for '{nodeName}'"); +logger.LogError(ex, "[GetOrcaHelloNodeAsync] Error retrieving node info for '{NodeName}'", nodeName); -logger.LogError(ex, $"[GetModelThresholdsAsync] Error retrieving ConfigMap for namespace '{namespaceName}'"); +logger.LogError(ex, "[GetModelThresholdsAsync] Error retrieving ConfigMap for namespace '{NamespaceName}'", namespaceName); -logger.LogError(ex, $"Exception in UpdateOrcaHelloDataAsync"); +logger.LogError(ex, "Exception in UpdateOrcaHelloDataAsync");Also applies to: 155-155, 353-353, 450-450, 467-467, 476-476, 573-573, 720-720, 770-770, 821-821, 898-898, 944-944
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@OrcanodeMonitor/Core/OrcaHelloFetcher.cs` around lines 87 - 101, The LogError calls in CreateK8sClient (and other places) use string interpolation which triggers CA2254; for static messages remove the leading '$' so the template is a constant, and for messages that include variables (e.g., the checks around k8sCACert, host, accessToken) convert the interpolated string into a message template with named placeholders and pass the variable(s) as separate arguments to logger.LogError instead of embedding them via $"..."; update all logger.LogError usages in OrcaHelloFetcher.cs (including the shown calls and the other mentioned lines) to follow this pattern so templates are constant and parameters are supplied separately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@OrcanodeMonitor/Core/OrcaHelloFetcher.cs`:
- Around line 350-355: Remove the stale comment inside the catch of
GetPodCommandOutput and update the logger call to a clear, accurate message
about failing to execute the pod command (not "retrieving node info");
specifically modify the catch in GetPodCommandOutput to delete the comment "//
Optionally log the exception here if logging is available" and change the
logger.LogError(ex, $"[GetPodCommandOutput] Error retrieving node info for
'{namespaceName}'") to a message that references executing the pod command (e.g.
"[GetPodCommandOutput] Error executing pod command for namespace
'{namespaceName}' and pod '{podName}'") while preserving passing the exception
object to logger.LogError.
In `@Test/OrcaHelloFetcherTests.cs`:
- Around line 15-34: CreateConsoleLogger instantiates _loggerFactory on each
TestsInitialize run but never disposes it; add a cleanup to release the
LoggerFactory after each test by implementing a TestCleanup method (or disposing
in existing cleanup) that calls _loggerFactory?.Dispose() (or
_loggerFactory?.DisposeAsync() if you choose async), ensuring you reference the
same _loggerFactory field and keep TestsInitialize/CreateConsoleLogger
unchanged.
---
Outside diff comments:
In `@OrcanodeMonitor/Core/OrcaHelloFetcher.cs`:
- Around line 464-478: GetOrcaHelloPodAsync currently logs errors when
_k8sClient is null and when GetBestPodStatus yields no bestPod, which is
inconsistent with other methods; change the early-exit branches in
GetOrcaHelloPodAsync so they mirror the others by removing the logger.LogError
calls (or demoting them to Debug/Trace) and simply return null when _k8sClient
is null or bestPod is null after calling GetBestPodStatus, keeping the rest of
the method unchanged.
---
Nitpick comments:
In `@OrcanodeMonitor/Core/OrcaHelloFetcher.cs`:
- Around line 87-101: The LogError calls in CreateK8sClient (and other places)
use string interpolation which triggers CA2254; for static messages remove the
leading '$' so the template is a constant, and for messages that include
variables (e.g., the checks around k8sCACert, host, accessToken) convert the
interpolated string into a message template with named placeholders and pass the
variable(s) as separate arguments to logger.LogError instead of embedding them
via $"..."; update all logger.LogError usages in OrcaHelloFetcher.cs (including
the shown calls and the other mentioned lines) to follow this pattern so
templates are constant and parameters are supplied separately.
In `@Test/OrcaHelloFetcherTests.cs`:
- Around line 18-26: The CreateConsoleLogger method in OrcaHelloFetcherTests
duplicates FetcherTests.CreateConsoleLogger; extract a shared static helper
(e.g., TestLoggerHelper.CreateConsoleLogger<T>()) that builds a LoggerFactory
with builder.AddConsole() and SetMinimumLevel(LogLevel.Debug) and returns
CreateLogger<T>(), then replace both FetcherTests.CreateConsoleLogger and
OrcaHelloFetcherTests.CreateConsoleLogger to call
TestLoggerHelper.CreateConsoleLogger<T>() (use the test class type as T) and
remove the duplicated methods.
Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
OrcanodeMonitor/Core/Fetcher.cs (1)
827-833:⚠️ Potential issue | 🟡 MinorWrong method name in exception log message.
The
catchinGetExactAudioSampleAsynclogs"Exception in UpdateManifestTimestampAsync"— a copy of the caller's name rather than the actual method. This misdirects debugging.🐛 Proposed fix
- logger.LogError(ex, "Exception in UpdateManifestTimestampAsync"); + logger.LogError(ex, "Exception in GetExactAudioSampleAsync");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@OrcanodeMonitor/Core/Fetcher.cs` around lines 827 - 833, The catch block inside GetExactAudioSampleAsync currently logs the wrong method name ("Exception in UpdateManifestTimestampAsync"); update the logger.LogError call in the catch within GetExactAudioSampleAsync to reference the correct method name and context (e.g., "Exception in GetExactAudioSampleAsync") so error messages point to the actual failing method; preserve passing the exception object (ex) and any existing contextual parameters to the logger.LogError call.
🧹 Nitpick comments (1)
Test/OrcaHelloFetcherTests.cs (1)
18-26:CreateConsoleLoggeris duplicated acrossFetcherTestsandOrcaHelloFetcherTests.The implementation is identical save for the generic type parameter. Consider extracting it to a shared test utility (e.g.,
TestLoggerHelper.CreateConsoleLogger<T>()) to reduce duplication.♻️ Proposed refactor — shared helper
// Test/TestLoggerHelper.cs (new file) internal static class TestLoggerHelper { public static (ILogger<T>, ILoggerFactory) CreateConsoleLogger<T>() { var factory = LoggerFactory.Create(b => { b.AddConsole(); b.SetMinimumLevel(LogLevel.Debug); }); return (factory.CreateLogger<T>(), factory); } }Then in each test class:
- private ILogger<OrcaHelloFetcherTests> CreateConsoleLogger() - { - _loggerFactory = LoggerFactory.Create(builder => - { - builder.AddConsole(); - builder.SetMinimumLevel(LogLevel.Debug); - }); - return _loggerFactory.CreateLogger<OrcaHelloFetcherTests>(); - } + // In TestsInitialize: + (_logger, _loggerFactory) = TestLoggerHelper.CreateConsoleLogger<OrcaHelloFetcherTests>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Test/OrcaHelloFetcherTests.cs` around lines 18 - 26, Extract the duplicated CreateConsoleLogger method from OrcaHelloFetcherTests and FetcherTests into a shared test utility (e.g., create internal static TestLoggerHelper with a generic CreateConsoleLogger<T>() that returns both ILogger<T> and ILoggerFactory), update OrcaHelloFetcherTests and FetcherTests to call TestLoggerHelper.CreateConsoleLogger<T>() and use the returned ILogger<T> (and preserve/assign the ILoggerFactory so tests can dispose it if needed), and remove the local CreateConsoleLogger implementations to eliminate duplication while keeping the same console logger configuration (AddConsole, SetMinimumLevel Debug).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@OrcanodeMonitor/Core/OrcaHelloFetcher.cs`:
- Around line 463-468: GetOrcaHelloPodAsync currently logs an error when
_k8sClient is null, which is inconsistent with GetOrcaHelloNodeAsync,
UpdateOrcaHelloDataAsync, FetchPodMetricsAsync and FetchNodeMetricsAsync that
silently return; change the null-client guard in GetOrcaHelloPodAsync (the block
referencing IKubernetes? client = _k8sClient and
logger.LogError("[GetOrcaHelloPodAsync] Kubernetes client is null")) to match
peers by either removing the LogError and just returning null or downgrading to
logger.LogWarning, so tests like
GetOrcaHelloPodAsync_ReturnsNull_WhenClientIsNull no longer produce spurious
error output; ensure the change is limited to GetOrcaHelloPodAsync to keep
behavior consistent across methods.
---
Outside diff comments:
In `@OrcanodeMonitor/Core/Fetcher.cs`:
- Around line 827-833: The catch block inside GetExactAudioSampleAsync currently
logs the wrong method name ("Exception in UpdateManifestTimestampAsync"); update
the logger.LogError call in the catch within GetExactAudioSampleAsync to
reference the correct method name and context (e.g., "Exception in
GetExactAudioSampleAsync") so error messages point to the actual failing method;
preserve passing the exception object (ex) and any existing contextual
parameters to the logger.LogError call.
---
Duplicate comments:
In `@OrcanodeMonitor/Core/OrcaHelloFetcher.cs`:
- Around line 350-354: The catch block in GetPodCommandOutput logs a misleading
message about "retrieving node info"; update the logger.LogError call inside the
GetPodCommandOutput method to use an accurate message (e.g., "Error executing
command in pod" or similar) and retain the exception and contextual variables
(namespaceName and any podName/command identifiers available) so the log
reflects the actual operation and context.
---
Nitpick comments:
In `@Test/OrcaHelloFetcherTests.cs`:
- Around line 18-26: Extract the duplicated CreateConsoleLogger method from
OrcaHelloFetcherTests and FetcherTests into a shared test utility (e.g., create
internal static TestLoggerHelper with a generic CreateConsoleLogger<T>() that
returns both ILogger<T> and ILoggerFactory), update OrcaHelloFetcherTests and
FetcherTests to call TestLoggerHelper.CreateConsoleLogger<T>() and use the
returned ILogger<T> (and preserve/assign the ILoggerFactory so tests can dispose
it if needed), and remove the local CreateConsoleLogger implementations to
eliminate duplication while keeping the same console logger configuration
(AddConsole, SetMinimumLevel Debug).
Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit