Enhancing Test Case Granularity for testGetLocalDateTimeTypes
#2360
Conversation
|
@microsoft-github-policy-service agree |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2360 +/- ##
============================================
+ Coverage 50.03% 50.25% +0.22%
- Complexity 3785 3863 +78
============================================
Files 143 145 +2
Lines 33204 33360 +156
Branches 5629 5654 +25
============================================
+ Hits 16613 16766 +153
- Misses 14207 14218 +11
+ Partials 2384 2376 -8 ☔ View full report in Codecov by Sentry. |
| private LocalDateTime convertOffsetDateTimeToLocalDateTime(OffsetDateTime value) { | ||
| return value.atZoneSameInstant(ZoneId.systemDefault()).toLocalDateTime(); | ||
| } | ||
|
|
There was a problem hiding this comment.
I'm not opposed to splitting the tests but refactoring into these methods seem unnecessary since they are only called once, why not just include them in the tests themselves?
There was a problem hiding this comment.
Thanks for the feedback! The decision to refactor was driven by enhancing test readability and making future maintenance easier. This approach will improve the clarity of our test cases and reduces debugging rerun time when debugging for specific scenario. When tests are more granular and focused, identifying and addressing issues becomes faster, leading to more efficient debugging cycles.
Even though the methods are called only once now, this structure allows us to easily expand or modify tests with minimal changes. The reduction in debugging rerun time and the enhancement in clarity are pivotal, as they ensure that our test suite remains robust, facilitating quicker modifications.
There was a problem hiding this comment.
I don't think it makes it any easier to read. If this changes in the future it can be changed then for now there is no need to do unnecessary refactoring and extra method calls
There was a problem hiding this comment.
Thanks for suggestion! Your perspective on the overall development picture is very helpful. I agree that maintainability should be balanced with immediate clarity. Let's keep the tests straightforward as you've advised.
|
Hi @Codegass, We're closing this because of reasons explained above by @lilgreenbird. Additionally, separating tests can lead to an increased test time, and this is not something we want. Please feel free to comment below if you have any additional questions or concerns about your pull request. |
Description
As a newcomer to the mssql-jdbc community, I've been exploring the test suite and noticed an opportunity for minor improvement in the
testGetLocalDateTimeTypestest case. Currently, this test case covers two distinct scenarios related to handling DATETIMEOFFSET conversions: one with the default behavior and another where the offset is ignored. I propose splitting this into two separate test cases to enhance test granularity and clarity.Splitting the test allows each case to focus on a unique behavior, making it easier to pinpoint issues when one scenario fails without impacting the other. This approach not only aids in more precise problem identification across different scenarios but also reduces individual test execution time, which can be beneficial when debugging and resolving bugs.
Key Points
I hope these changes will make our testing suite more robust and user-friendly. I'm open to discussion and feedback on this proposal, as I'm eager to learn from the community and contribute positively.