Conversation
Hmnt39
force-pushed
the
hemant/logger_coverage
branch
from
7fab6d6 to
f3875c1
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #648 +/- ##
==========================================
+ Coverage 77.71% 77.92% +0.21%
==========================================
Files 136 136
Lines 13226 13226
Branches 1991 1991
==========================================
+ Hits 10278 10306 +28
+ Misses 2112 2082 -30
- Partials 836 838 +2 🚀 New features to boost your workflow:
|
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces new unit tests for CallbackUtil and expands the LoggingTest suite to verify different logger configurations and levels. Feedback was provided regarding a potential IOException on Windows in the new logging tests, where the test.log file is deleted before the ServiceProvider is disposed, which may cause file lock conflicts.
Hmnt39
added
the
waiting-for-risk-review
| } // provider is disposed here | ||
|
|
||
| // Cleanup test.log file if created | ||
| if (File.Exists("test.log")) |
There was a problem hiding this comment.
If logType = "file", you should Assert that the file exists, and then delete it if FileLogger creates a file on initialization. If it does not, then we don't need this block.
There was a problem hiding this comment.
Added the cleanup when config type is file
| switch (logType) | ||
| { | ||
| case "console": | ||
| services.AddSingleton(new LoggerConfig | ||
| { | ||
| Console = new ConsoleConfig { Level = "information" } | ||
| }); | ||
| break; | ||
| case "file": | ||
| services.AddSingleton(new LoggerConfig | ||
| { | ||
| File = new FileConfig { Level = "warning", Path = "test.log" } | ||
| }); | ||
| break; | ||
| case "trace-listener": | ||
| services.AddSingleton(new LoggerConfig | ||
| { | ||
| TraceListener = new TraceListenerConfig { Level = "error" } | ||
| }); | ||
| break; | ||
| } | ||
| services.AddLogger(); |
There was a problem hiding this comment.
This whole block is mostly duplicated with the test below, really should be a common method imo.
There was a problem hiding this comment.
add a common method for duplicated code
| Assert.NotNull(retrievedConfig.Console); | ||
| Assert.Equal(level, retrievedConfig.Console.Level); | ||
| break; | ||
| case "file": | ||
| Assert.NotNull(retrievedConfig.File); | ||
| Assert.Equal(level, retrievedConfig.File.Level); | ||
| break; | ||
| case "trace-listener": | ||
| Assert.NotNull(retrievedConfig.TraceListener); | ||
| Assert.Equal(level, retrievedConfig.TraceListener.Level); |
There was a problem hiding this comment.
This is only asserting the level of the logger configuration file, not the logger itself.
There was a problem hiding this comment.
Added assertion for logger and log level too.
Hmnt39
removed
the
waiting-for-risk-review
Hmnt39
added
the
waiting-for-risk-review
3 participants
This PR improves test coverage for the logging and callback utilities by adding unit tests.