[rush] Include seconds in generated change file names so repeated "rush change" runs do not collide#5818
Conversation
Running rush change twice in the same minute silently overwrote the change file from the first run, since generatePath only used minute granularity. Pass useSeconds so the filename includes seconds, and fix the useSeconds branch which only replaced the first colon. Fixes microsoft#2195
LeSingh1
requested review from
apostolisms,
bmiddha,
dmichon-msft,
iclanton,
jxanthony and
octogonz
as code owners
| { | ||
| "changes": [ | ||
| { | ||
| "comment": "Include seconds in the generated change file name so that running \"rush change\" more than once in the same minute no longer silently overwrites the previously generated change file.", |
There was a problem hiding this comment.
| "comment": "Include seconds in the generated change file name so that running \"rush change\" more than once in the same minute no longer silently overwrites the previously generated change file.", | |
| "comment": "Include seconds in the generated change file name so that running `rush change` more than once in the same minute no longer silently overwrites the previously generated change file.", |
| } | ||
| ], | ||
| "packageName": "@microsoft/rush", | ||
| "email": "sshaurya914@gmail.com" |
There was a problem hiding this comment.
| "email": "sshaurya914@gmail.com" | |
| "email": "LeSingh1@users.noreply.github.com" |
|
@LeSingh1 - the build on Windows failed. Can you investigate? May need to use fake timers in Jest. |
The Date constructor spy was bypassed by the V8 fast-path on Windows/Node
v24, so new Date().toJSON() returned the real wall-clock time instead of
the pinned value. Switch to jest.useFakeTimers().setSystemTime() which
intercepts at the engine level and is reliable across platforms.
Also fix the not.toContain(':') assertion to check only path.basename()
instead of the full path. On Windows the generated path starts with a
drive letter (e.g. "D:\...") which legitimately contains a colon and
was causing the assertion to fail even though the timestamp itself had
no colons.
Head branch was pushed to by a user without write access
|
Thanks for the pointer. I found two issues in the test that together caused the Windows failure. The Date constructor spy (jest.spyOn(global, 'Date').mockImplementation(...)) was being bypassed on Windows with Node.js v24. The V8 Date fast-path can skip the spy, so new Date().toJSON() returned the real wall-clock time instead of the pinned value. I switched to jest.useFakeTimers().setSystemTime() with jest.useRealTimers() in the finally block, which intercepts at the engine level and is reliable across all platforms. The second issue was the not.toContain(':') assertion. On Windows, path.join produces a path starting with a drive letter like D:..., so the full path always contains a colon regardless of the timestamp. I changed the assertion to run on path.basename(generatedPath) only, which is just the filename and has no drive letter. |
2 participants
Fixes #2195
When you run "rush change" twice within the same minute, the second run silently overwrites the change file written by the first one, so one of your messages gets lost. The generated filename only has minute-level granularity, which means both runs resolve to the same path. The issue thread confirms this: waiting a minute between invocations makes it behave correctly.
The cause is in ChangeFile.generatePath(), which called _getTimestamp() with no arguments. That formats the time as YYYY-MM-DD-HH-MM. The method already accepted a useSeconds parameter, but nothing ever passed it. This change passes true so the filename includes seconds (YYYY-MM-DD-HH-MM-SS), which is enough to keep separate invocations from colliding in normal usage.
While turning that on I noticed the useSeconds branch was itself broken. It used matches[2].replace(:, -), which only replaces the first colon, so it produced strings like 20-20:30 with a leftover colon. A colon is not even a valid filename character on Windows. I changed it to a global replace so every colon is converted.
I added a unit test in ChangeFile.test.ts that pins the branch name and the clock, then asserts the generated path contains the seconds and has no leftover colons. I confirmed the test fails against the previous behavior and passes with this change. I also added a change file under common/changes.