-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Playwright: add randomly generated integers as suffix for generated test files #55720
Conversation
…me, akin to `getBlogName`.
This PR does not affect the size of JS and CSS bundles shipped to the user's browser. Generated by performance advisor bot at iscalypsofastyet.com. |
Link to Calypso live: https://calypso.live?image=registry.a8c.com/calypso/app:build-13457 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Had two optional questions. Approved!
@@ -66,7 +66,8 @@ export function createTestFile( { | |||
sourceFileName: string; | |||
testFileName?: string; | |||
} ): string { | |||
let fileName = getTimestamp(); | |||
let fileName = `${ getTimestamp() }${ getRandomInteger( 100, 999 ) }`; | |||
console.log( fileName ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you want to keep the log in here? Or was this just for debugging? Either is fine, just wanted to double check :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in dcc4eb5.
@@ -66,7 +66,8 @@ export function createTestFile( { | |||
sourceFileName: string; | |||
testFileName?: string; | |||
} ): string { | |||
let fileName = getTimestamp(); | |||
let fileName = `${ getTimestamp() }${ getRandomInteger( 100, 999 ) }`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about this method of generating names, and wondered: do we want to separate the integer from the timestamp piece? In case we need to make sense of that timestamp piece later?
In other words, it would be something like: ${ getTimestamp() }x${ getRandomInteger( 100, 999 ) }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
${ getTimestamp() }-${ getRandomInteger( 100, 999 ) }
(a hyphen) would work well with the platform, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed in dcc4eb5.
On hold as #55678 is merged and as files are now generated asynchronously, it should help avoid conflicts. |
I've incorporated changes proposed here into #55693. |
Changes proposed in this Pull Request
This PR proposes to increase uniqueness of the file name of generated test files by adding a pseudo-random suffix ranging from 100-999.
Testing instructions
Related to #55717, #55674.