Handle Carbon 3 timestamp parsing and add boundary tests#1132
Conversation
dati18
changed the title
…ling Bug: T426592
dati18
force-pushed
the
carbon3-tests-update
branch
from
89a0590 to
069c041
Compare
| public static function getCarbonFromMWTimestamp(string $MWTimestamp): CarbonImmutable { | ||
| $carbon = CarbonImmutable::createFromFormat(self::MWTimestampFormat, $MWTimestamp); | ||
| if ($carbon === null) { | ||
| try { |
There was a problem hiding this comment.
I'm a bit unclear about this part.
- since Carbon 3 will throw an exception when
createFormFormat, I'm not certain it helps to catch it just so we can throw our own. This could even make it harder to spot the reason for why it failed. - the second check also seems obsolete since we either get a
CarbonImmutableor an Exception gets thrown
So I think this method could be as simple as just line 19 - if something goes wrong an exception gets thrown as it used to. Is there more context for this additional logic that I'm not aware of?
There was a problem hiding this comment.
you're right to question it, it's an overthinking part from me. My logic is createFromFormat() may throw a non-instance (null/false), so an extra instanceof guard prevents type error when returning $carbon.
|
|
||
| public function testGetCarbonFromMWTimestampWithInvalidTimestamp() { | ||
| $this->expectException(InvalidFormatException::class); | ||
| $this->expectExceptionMessage('Unable to create Carbon object'); |
There was a problem hiding this comment.
As line 21 is expecting the Exception already, I struggle to see how this check helps us
There was a problem hiding this comment.
I just want to lock down the exact error message. a bit redundant, yes... WHat do you think? keep it or remove it?
|
The tighter tests look good to me overall |
2 participants
MWTimestampHelperTest.php: Added coverage to ensure invalid MediaWiki timestamps are normalized into the helper’s expected InvalidFormatException behavior under Carbon 3.SendEmptyWikiNotificationsJobTest.php: Tightened notification-threshold coverage so a wiki that is almost old enough (for example: 29.5 days old) does not trigger an empty-wiki notification early.PlatformStatsSummaryJobTest.php: Added a test to ensure a second-precisionlastEdittimestamp exactly at the inactivity cutoff is still counted as active.ConversionMetricTest.php: Added a test to verify fractional day diffs are truncated to the integer values expected by the conversion metrics API.Bug: T426592