Refactor test script for improved readability and modularity #1956
Conversation
|
Thanks for the PR! This section of the codebase is owned by @saschanaz - if they write a comment saying "LGTM" then it will be merged. |
Sure |
|
Done @saschanaz |
src/test.ts
Outdated
| function normalizeLineEndings(text: string): string { | ||
| return text.replace(/\r\n?/g, "\n"); | ||
| function normalizeLineEndings(text: string) { | ||
| text.replace(/\r\n?/g, "\n"); |
src/test.ts
Outdated
| try { | ||
| outputFiles = fs.readdirSync(outputFolder); | ||
| return normalizeLineEndings(fs.readFileSync(filePath, "utf-8")); |
There was a problem hiding this comment.
We already stat the file before calling this and thus doesn't expect any error here. Thus we should skip try-catch here to see actual failures if happens.
src/test.ts
Outdated
| const childBaselineFolder = new URL(`${file}/`, baselineFolder); | ||
| const childOutputFolder = new URL(`${file}/`, outputFolder); | ||
| if (!compareToBaselines(childBaselineFolder, childOutputFolder)) { | ||
| if (fs.existsSync(baselinePath) || fs.existsSync(outputPath)) { |
There was a problem hiding this comment.
This is not same as detecting directory?
src/test.ts
Outdated
| } | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| function compileGeneratedFiles(lib: string, ...files: string[]) { | ||
| function compileGeneratedFiles(lib: string[] | string, ...files: string[]) { |
There was a problem hiding this comment.
But is lib ever an array here?
src/test.ts
Outdated
| console.error(e.stdout.toString()); | ||
| console.error(); |
There was a problem hiding this comment.
Skipping this would hide the actual error which would make debugging harder.
|
I have fixed them @saschanaz |
|
Hello @saschanaz |
|
Why do we need to do this? This just seems like a plain refactor, but no behavior is changing. Just seems like churn? |
This is just a refactor, and making the code more readable and easy to maintain. |
|
Personally, I would not try and work on this kind of change; it means us maintainers have to review PRs to see what they're doing, if they contain any new bugs, for code which theoretically is already working fine. Other improvements are of course welcome, but for plain code changes like this, I don't really think it's worth the time spent compared to other changes (e.g., you've sent some good other ones!). |
|
It's indeed a bit hard to make sure all changes are correct, would be much easier to check when each change is done separately. |
src/test.ts
Outdated
| ) | ||
| : null; | ||
| const isBaselineFile = | ||
| fs.existsSync(baselinePath) && fs.statSync(baselinePath).isFile(); |
There was a problem hiding this comment.
It's actually advised to not call exists before other calls, because things can potentially be racy and the file can go away after that call.
https://nodejs.org/docs/latest/api/fs.html#fsexistspath-callback
Using fs.exists() to check for the existence of a file before calling fs.open(), fs.readFile(), or fs.writeFile() is not recommended. Doing so introduces a race condition, since other processes may change the file's state between the two calls. Instead, user code should open/read/write the file directly and handle the error raised if the file does not exist.
|
No problem. I will close this one |
|
I don't think this should be thrown away; just splitting this would be fine. |
|
I have made it smaller @saschanaz |
Co-authored-by: Kagami Sascha Rosylight <[email protected]>
|
LGTM |
|
There was an issue merging, maybe try again saschanaz. Details |
This PR refactors the test script to enhance maintainability, readability, and efficiency.
Changes:
getFiles,readFileContent) to reduce redundancy.compileSetsarray to streamline and organize compilation tasks dynamically.Benefits: