fix(scripts): centralize ISO 8601 timestamp regex in CIHelpers#1343
fix(scripts): centralize ISO 8601 timestamp regex in CIHelpers#1343MauroDruwel wants to merge 5 commits intomicrosoft:mainfrom
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
katriendg
left a comment
katriendg
left a comment
There was a problem hiding this comment.
Lovely to have you pick up an issue from the backlog @MauroDruwel 🙏 It's looking all good to me!
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1343 +/- ##
==========================================
- Coverage 87.66% 87.65% -0.01%
==========================================
Files 61 61
Lines 9328 9329 +1
==========================================
Hits 8177 8177
- Misses 1151 1152 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
rezatnoMsirhC
left a comment
rezatnoMsirhC
left a comment
There was a problem hiding this comment.
Thank you for your contribution! Overall things look pretty neat, but I left a few comments to ensure we maintain regression detection.
| function Get-StandardTimestampPattern { | ||
| <# | ||
| .SYNOPSIS | ||
| Returns the regex pattern that matches Get-StandardTimestamp output. | ||
|
|
||
| .DESCRIPTION | ||
| Returns a single-source regex anchored to the ISO 8601 round-trip format | ||
| produced by Get-StandardTimestamp (e.g. "2025-01-15T18:30:00.0000000Z"). | ||
| Use this function in tests instead of hard-coding the pattern so that all | ||
| assertions stay in sync when the timestamp format changes. | ||
|
|
||
| .OUTPUTS | ||
| System.String - Anchored regex pattern for ISO 8601 UTC timestamps. | ||
| #> | ||
| [CmdletBinding()] | ||
| [OutputType([string])] | ||
| param() | ||
|
|
||
| return '^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d+Z$' | ||
| } | ||
|
|
||
| Export-ModuleMember -Function @( | ||
| 'Get-StandardTimestamp', | ||
| 'Get-StandardTimestampPattern', |
There was a problem hiding this comment.
nitpick: Does this need to be a function? There are no parameters and no logic in this function, so the StandardTimestampPattern could be exposed as an exported module variable instead. Something like this could work:
# Alternative: module-scoped readonly variable
$script:StandardTimestampPattern = '^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d+Z$'
...
Export-ModuleMember -Variable @('StandardTimestampPattern')
# Callers use: $StandardTimestampPattern
|
|
||
| It 'Matches ISO 8601 UTC format ending in Z' { | ||
| Get-StandardTimestamp | Should -Match '^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d+Z$' | ||
| Get-StandardTimestamp | Should -Match (Get-StandardTimestampPattern) |
There was a problem hiding this comment.
This is an anti-pattern. While code-reuse is usually elegant and highly-encouraged, it can also be counterproductive to reuse non-test code/constants in tests.
This article provide a nice example of how things can go wrong.
For example, if Get-StandardTimestampPattern were updated to return a more generous pattern like .*Z, this test would still pass, and therefore we wouldn't be able to rely on this unit test case to help us detect the regression.
I recommend reverting this so we can better detect behavior drifts for Get-StandardTimestamp, especially since this is the module which owns the pattern's correctness. It's probably fine for other tests like those for SecurityHelpers to keep using Get-StandardTimestampPattern:
| Get-StandardTimestamp | Should -Match (Get-StandardTimestampPattern) | |
| Get-StandardTimestamp | Should -Match '^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d+Z$' |
| Describe 'Get-StandardTimestampPattern' -Tag 'Unit' { | ||
| It 'Returns a non-empty string' { | ||
| Get-StandardTimestampPattern | Should -Not -BeNullOrEmpty | ||
| } | ||
|
|
||
| It 'Pattern matches Get-StandardTimestamp output' { | ||
| $timestamp = Get-StandardTimestamp | ||
| $pattern = Get-StandardTimestampPattern | ||
| $timestamp | Should -Match $pattern | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Neither test will catch if the pattern is changed to something too permissive or too restrictive. A proper test suite for Get-StandardTimestampPattern should include positive and negative cases using inline literals because this is the function that owns the pattern's correctness.
| Describe 'Get-StandardTimestampPattern' -Tag 'Unit' { | |
| It 'Returns a non-empty string' { | |
| Get-StandardTimestampPattern | Should -Not -BeNullOrEmpty | |
| } | |
| It 'Pattern matches Get-StandardTimestamp output' { | |
| $timestamp = Get-StandardTimestamp | |
| $pattern = Get-StandardTimestampPattern | |
| $timestamp | Should -Match $pattern | |
| } | |
| } | |
| Describe 'Get-StandardTimestampPattern' -Tag 'Unit' { | |
| It 'Returns a non-empty string' { | |
| Get-StandardTimestampPattern | Should -Not -BeNullOrEmpty | |
| } | |
| It 'Matches a valid ISO 8601 UTC timestamp' { | |
| '2026-04-16T14:55:00.0000000Z' | Should -Match (Get-StandardTimestampPattern) | |
| } | |
| It 'Does not match a timestamp missing fractional seconds' { | |
| '2026-04-16T14:55:00Z' | Should -Not -Match (Get-StandardTimestampPattern) | |
| } | |
| It 'Does not match a local time without Z suffix' { | |
| '2026-04-16T14:55:00.0000000+00:00' | Should -Not -Match (Get-StandardTimestampPattern) | |
| } | |
| It 'Pattern matches Get-StandardTimestamp output' { | |
| $timestamp = Get-StandardTimestamp | |
| $pattern = Get-StandardTimestampPattern | |
| $timestamp | Should -Match $pattern | |
| } | |
| } |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thanks so much for the careful review, @rezatnoMsirhC. Really appreciate the feedback here. I pushed an update that addresses your points:
Would love a second look when you have time. |
5 participants
Pull Request
Description
Add
Get-StandardTimestampPatterntoCIHelpers.psm1returning the canonicalregex for validating
Get-StandardTimestampoutput. Replace all standalonehardcoded ISO 8601 timestamp patterns across five test files with calls to this
function, eliminating duplication and drift.
Three distinct regex variants had already emerged across the test suite:
\.\d+Z$\.\d{3,7}Z$.*Z$Z, defeats validation purposeThe new function returns a single source of truth so all assertions stay in sync
automatically if the timestamp format changes.
Relationship to #1314
This PR was born from the review discussion on #1314, filed separately as #1333.
timestamp→TimestampCIHelpers.psm1+ 5 test filesBoth PRs are needed — they address different layers of the same standardization
effort (#1003). #1314 fixes how timestamps are named in JSON output; this PR
fixes how timestamps are validated in tests. Neither blocks the other and they
can merge in any order without conflicts.
Related Issue(s)
Closes #1333
Type of Change
Code & Documentation:
Other:
.ps1,.sh,.py)Testing
(Get-StandardTimestampPattern)calls in 5 test filesCIHelpers.Tests.ps1:Get-StandardTimestampPatternreturns a non-empty stringGet-StandardTimestampoutput (cross-validation)CIHelpersmodule imports to 3 test files that lacked directaccess (
SecurityClasses,SecurityHelpers,FrontmatterValidation)Files Changed
Module (new function + export):
scripts/lib/Modules/CIHelpers.psm1—Get-StandardTimestampPatternfunction andExport-ModuleMemberentryTests (hardcoded regex → function call):
scripts/tests/lib/CIHelpers.Tests.ps1— replaced regex; addedDescribe 'Get-StandardTimestampPattern'blockscripts/tests/linting/FrontmatterValidation.Tests.ps1— replaced 2 regexes; added CIHelpers importscripts/tests/linting/Invoke-YamlLint.Tests.ps1— replaced 1 regex (already imported CIHelpers)scripts/tests/security/SecurityClasses.Tests.ps1— replaced 1 regex; added CIHelpers importscripts/tests/security/SecurityHelpers.Tests.ps1— replaced 1 regex; added CIHelpers importChecklist
Required Checks
Required Automated Checks
npm run lint:mdnpm run spell-checknpm run lint:frontmatternpm run validate:skillsnpm run lint:md-linksnpm run lint:psnpm run plugin:generatenpm run docs:testSecurity Considerations