Skip to content

[core] Add unit tests for TDatime - #23510

Closed
ravindra-RKB wants to merge 1 commit into
root-project:masterfrom
ravindra-RKB:add-tdatime-tests
Closed

ravindra-RKB wants to merge 1 commit into
root-project:masterfrom
ravindra-RKB:add-tdatime-tests

Conversation

@ravindra-RKB

Copy link
Copy Markdown
Contributor

This PR introduces unit tests for the TDatime class in core/base/test.

Testing the time construction and modification ensures that TDatime constructors and Set methods work correctly under various conditions. This provides better test coverage for the base module and helps prevent future regressions related to date and time handling.

This commit introduces unit tests for the TDatime class to ensure the correct behavior of its constructors and Set methods.
@guitargeek

Copy link
Copy Markdown
Contributor

I don't see the motivation for adding these random tests. It's true that we have many lines of code not covered by tests, but trying to add tests for everything in retrospect introduces way too much code churn.

Our policy is that if code is changed in a non-trivial way, you should add a test to go with it. But sprinkling some new unit tests into an existing decades old code base is not really worth it.

It's fine for you if I close this PR and your other one?

@ravindra-RKB

Copy link
Copy Markdown
Contributor Author

Hi @guitargeek,

Thank you for reviewing the PR and explaining the project's testing policy. I completely understand the concern about code churn in retrospect.

Yes, please feel free to close this PR as well as the other one (#23511). In the future, I will make sure to add tests only when making non-trivial changes to the codebase as per the guidelines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants