Add round-trip and leap-year coverage tests for DateTimeConverter (pre/post 1970)#135
Open
Rutuja-Patil-Bosch wants to merge 32 commits into
Open
Add round-trip and leap-year coverage tests for DateTimeConverter (pre/post 1970)#135Rutuja-Patil-Bosch wants to merge 32 commits into
Rutuja-Patil-Bosch wants to merge 32 commits into
Conversation
…lse_isValidDateTimeFormat EpochToDateTime_Before1970_LeapYear_January_AdjustsDaysSum invalid_day_values invalid_month_values
removed redundnat checks
Feature: bgsw_baselib_ut
|
The created documentation from the pull request is available at: docu-html |
gierer
suggested changes
Apr 7, 2026
Member
|
@Rutuja-Patil-Bosch please avoid bot based commits. Feel free to contact me directly. |
Open
Member
|
@gierer, please have a look at this PR |
gierer
suggested changes
May 18, 2026
There was a problem hiding this comment.
Pull request overview
This PR adds additional unit tests for score::common::DateTimeConverter to increase coverage around invalid date inputs and leap-year / pre-/post-1970 conversion edge cases.
Changes:
- Added tests for invalid month/day values in
isValidDateTimeFormat. - Added a test ensuring
dateTimeToEpochreturnsfalseand does not mutate the output epoch when the input is invalid. - Added a few DateTime→epoch→DateTime “round-trip” tests targeting 1968 and 1972 leap-year boundaries.
Comments suppressed due to low confidence (3)
score/datetime_converter/datetime_converter_test.cpp:526
- Same issue here: the test returns early if
dateTimeToEpochfails, which makes the test pass without validating anything. Use an assertion so failures are visible and the remainder of the test always executes when appropriate.
time_t epoch{};
auto dateTimeOriginal =
std::make_shared<DateTimeType>(1972, 12, 31, 0, 0, 1);
if (!score::common::dateTimeToEpoch(dateTimeOriginal, &epoch))
return;
auto dateTimeConverted = score::common::epochToDateTime(epoch);
score/datetime_converter/datetime_converter_test.cpp:530
- The assertion expects the year to change from 1972 to 1973 after a DateTime→epoch→DateTime round-trip, which contradicts the test description (“remain unchanged”) and will lock in incorrect behavior. The round-trip expectation should match the original datetime fields.
auto dateTimeConverted = score::common::epochToDateTime(epoch);
ASSERT_NE(dateTimeConverted, nullptr);
//After 1970 year is adjusted based on daysome
ASSERT_EQ(dateTimeConverted->m_year, 1973);
score/datetime_converter/datetime_converter_test.cpp:546
- This test expects
epochToDateTimeto return nullptr for a valid date (1968-01-01) even thoughdateTimeToEpochsucceeded, which would encode a conversion bug rather than catch it. If this is intended behavior, it should be documented at the API level; otherwise the test should assert that the round-trip returns a valid DateTime equal to the input.
RecordProperty("Description",
"Verify that conversion from DateTime to Epoch to DateTime for pre-1970 leap year boundary (1968-01-01) results in an invalid DateTime, returning nullptr due to daysSum adjustment.");
RecordProperty("TestType", "requirements-based");
time_t epoch{};
auto d = std::make_shared<DateTimeType>(1968, 1, 1, 0, 0, 1);
ASSERT_TRUE(score::common::dateTimeToEpoch(d, &epoch));
auto dt = score::common::epochToDateTime(epoch);
//InvalidDateTimeFormat due to day some adjustment & returns nullptr
EXPECT_EQ(dt, nullptr);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+503
to
+508
| time_t epoch{}; | ||
| auto dateTimeOriginal = | ||
| std::make_shared<DateTimeType>(1968, 12, 31, 0, 0, 1); | ||
| if (!score::common::dateTimeToEpoch(dateTimeOriginal, &epoch)) | ||
| return; | ||
| auto dateTimeConverted = score::common::epochToDateTime(epoch); |
Comment on lines
+504
to
+511
| auto dateTimeOriginal = | ||
| std::make_shared<DateTimeType>(1968, 12, 31, 0, 0, 1); | ||
| if (!score::common::dateTimeToEpoch(dateTimeOriginal, &epoch)) | ||
| return; | ||
| auto dateTimeConverted = score::common::epochToDateTime(epoch); | ||
| ASSERT_NE(dateTimeConverted, nullptr); | ||
| ASSERT_EQ(dateTimeConverted->m_year, 1968); | ||
| } |
Comment on lines
+528
to
+545
| //After 1970 year is adjusted based on daysome | ||
| ASSERT_EQ(dateTimeConverted->m_year, 1973); | ||
|
|
||
| } | ||
|
|
||
| TEST_F(DateTimeConverterTest, EpochToDateTime_Pre1970_LeapYear_Jan1_InvalidConversion_ReturnsNull) | ||
| { | ||
| RecordProperty("Description", | ||
| "Verify that conversion from DateTime to Epoch to DateTime for pre-1970 leap year boundary (1968-01-01) results in an invalid DateTime, returning nullptr due to daysSum adjustment."); | ||
| RecordProperty("TestType", "requirements-based"); | ||
|
|
||
| time_t epoch{}; | ||
| auto d = std::make_shared<DateTimeType>(1968, 1, 1, 0, 0, 1); | ||
| ASSERT_TRUE(score::common::dateTimeToEpoch(d, &epoch)); | ||
|
|
||
| auto dt = score::common::epochToDateTime(epoch); | ||
| //InvalidDateTimeFormat due to day some adjustment & returns nullptr | ||
| EXPECT_EQ(dt, nullptr); |
Comment on lines
+522
to
+530
|
|
||
| if (!score::common::dateTimeToEpoch(dateTimeOriginal, &epoch)) | ||
| return; | ||
|
|
||
| auto dateTimeConverted = score::common::epochToDateTime(epoch); | ||
| ASSERT_NE(dateTimeConverted, nullptr); | ||
| //After 1970 year is adjusted based on daysome | ||
| ASSERT_EQ(dateTimeConverted->m_year, 1973); | ||
|
|
Comment on lines
+498
to
+535
| TEST_F(DateTimeConverterTest, EpochToDateTime_Pre1970_LeapYear_DaysSumDecrementedInJanuary) | ||
| { | ||
| RecordProperty("Description", "Verify that datetime values remain unchanged after roundtrip conversion (DateTime to Epoch to DateTime) for pre 1970 leap years."); | ||
| RecordProperty("TestType", "requirements-based"); | ||
|
|
||
| time_t epoch{}; | ||
| auto dateTimeOriginal = | ||
| std::make_shared<DateTimeType>(1968, 12, 31, 0, 0, 1); | ||
| if (!score::common::dateTimeToEpoch(dateTimeOriginal, &epoch)) | ||
| return; | ||
| auto dateTimeConverted = score::common::epochToDateTime(epoch); | ||
| ASSERT_NE(dateTimeConverted, nullptr); | ||
| ASSERT_EQ(dateTimeConverted->m_year, 1968); | ||
| } | ||
|
|
||
| TEST_F(DateTimeConverterTest, EpochToDateTime_Post1970_LeapYear_DaysSumDecremented) | ||
| { | ||
| RecordProperty("Description", | ||
| "Verify that datetime values remain unchanged after roundtrip conversion (DateTime to Epoch to DateTime) for post-1970 leap years."); | ||
| RecordProperty("TestType", "requirements-based"); | ||
|
|
||
| time_t epoch{}; | ||
| auto dateTimeOriginal = | ||
| std::make_shared<DateTimeType>(1972, 12, 31, 0, 0, 1); | ||
|
|
||
| if (!score::common::dateTimeToEpoch(dateTimeOriginal, &epoch)) | ||
| return; | ||
|
|
||
| auto dateTimeConverted = score::common::epochToDateTime(epoch); | ||
| ASSERT_NE(dateTimeConverted, nullptr); | ||
| //After 1970 year is adjusted based on daysome | ||
| ASSERT_EQ(dateTimeConverted->m_year, 1973); | ||
|
|
||
| } | ||
|
|
||
| TEST_F(DateTimeConverterTest, EpochToDateTime_Pre1970_LeapYear_Jan1_InvalidConversion_ReturnsNull) | ||
| { | ||
| RecordProperty("Description", |
Comment on lines
+498
to
+502
| TEST_F(DateTimeConverterTest, EpochToDateTime_Pre1970_LeapYear_DaysSumDecrementedInJanuary) | ||
| { | ||
| RecordProperty("Description", "Verify that datetime values remain unchanged after roundtrip conversion (DateTime to Epoch to DateTime) for pre 1970 leap years."); | ||
| RecordProperty("TestType", "requirements-based"); | ||
|
|
…c since daysSum is normalized later. Fixed off-by-one by changing >= to >. Removed related test cases.
Baselib ut code update & relavant test case changes 1. New common function (daysInMonth()) added to get total number of days in month. This will be used at multiple places in code. 2. epochToDateTime() api is update to get correct date time values as discussed in PR eclipse-score#135 3. Accordingly redundant test cases are removed & for updated code test cas has been added
| } | ||
|
|
||
| return true; | ||
| return (dateTime->m_day <= daysInMonth(dateTime->m_year, dateTime->m_month)); |
| EXPECT_EQ(dateTimeConverted->m_second, 0); | ||
| } | ||
|
|
||
| TEST_F(DateTimeConverterTest, EpochToDateTime_Pre1800_ReturnsNull) |
| namespace common | ||
| { | ||
|
|
||
| std::int8_t daysInMonth(const std::int16_t year, const std::int8_t month) |
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
This PR extends unit test coverage for DateTimeConverter along with necessary code change by adding round-trip validation and additional leap-year scenarios, especially around pre-1970 and post-1970 boundaries.
Changes
Motivation
The added tests ensure correctness of:
This improves confidence in DateTimeConverter for:
Notes
Test Scope
Impact