Refactor test script for improved readability and modularity #1956

Bashamega · 2025-03-31T12:20:26Z

This PR refactors the test script to enhance maintainability, readability, and efficiency.

Changes:

Extracted reusable functions (getFiles, readFileContent) to reduce redundancy.
Simplified file comparison logic for better maintainability.
Introduced compileSets array to streamline and organize compilation tasks dynamically.
Improved error handling and logging for better debugging.

Benefits:

Cleaner code: Eliminates duplication and improves readability.
Easier maintenance: More modular and extensible structure.
Improved debugging: Enhanced error messages for easier troubleshooting.

github-actions · 2025-03-31T12:20:41Z

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.

saschanaz

First of all can you keep the existing function style instead of moving to const? A style change like that should be done with a good consensus and should be done for all files for consistency. And I don't think we have consensus.

Bashamega · 2025-04-01T07:17:02Z

First of all can you keep the existing function style instead of moving to const? A style change like that should be done with a good consensus and should be done for all files for consistency. And I don't think we have consensus.

Sure

Bashamega · 2025-04-01T07:26:04Z

Done @saschanaz

saschanaz · 2025-04-01T07:31:02Z

src/test.ts

-function normalizeLineEndings(text: string): string {
-  return text.replace(/\r\n?/g, "\n");
+function normalizeLineEndings(text: string) {
+  text.replace(/\r\n?/g, "\n");


saschanaz · 2025-04-01T07:32:46Z

src/test.ts

+function readFileContent(filePath: URL) {
  try {
-    outputFiles = fs.readdirSync(outputFolder);
+    return normalizeLineEndings(fs.readFileSync(filePath, "utf-8"));


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.

saschanaz · 2025-04-01T07:35:22Z

src/test.ts

-      const childBaselineFolder = new URL(`${file}/`, baselineFolder);
-      const childOutputFolder = new URL(`${file}/`, outputFolder);
-      if (!compareToBaselines(childBaselineFolder, childOutputFolder)) {
+    if (fs.existsSync(baselinePath) || fs.existsSync(outputPath)) {


This is not same as detecting directory?

saschanaz · 2025-04-01T07:36:39Z

src/test.ts

 }

-function compileGeneratedFiles(lib: string, ...files: string[]) {
+function compileGeneratedFiles(lib: string[] | string, ...files: string[]) {


But is lib ever an array here?

saschanaz · 2025-04-01T07:38:32Z

src/test.ts

-    console.error(e.stdout.toString());
-    console.error();


Skipping this would hide the actual error which would make debugging harder.

Bashamega · 2025-04-01T07:47:53Z

I have fixed them @saschanaz

Bashamega · 2025-04-03T07:58:09Z

Hello @saschanaz
Can you please review this? I have updated the PR

jakebailey · 2025-04-17T17:37:40Z

Why do we need to do this? This just seems like a plain refactor, but no behavior is changing. Just seems like churn?

Bashamega · 2025-04-17T17:43:21Z

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.

jakebailey · 2025-04-17T17:46:21Z

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!).

saschanaz · 2025-04-17T19:28:23Z

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.

saschanaz · 2025-04-17T19:32:19Z

src/test.ts

-        )
-      : null;
+    const isBaselineFile =
+      fs.existsSync(baselinePath) && fs.statSync(baselinePath).isFile();


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.

Bashamega · 2025-04-18T03:02:23Z

No problem. I will close this one

saschanaz · 2025-04-18T06:27:07Z

I don't think this should be thrown away; just splitting this would be fine.

Bashamega · 2025-04-18T06:45:46Z

I have made it smaller @saschanaz

saschanaz

Now this is what I call a great refactoring.

src/test.ts

Co-authored-by: Kagami Sascha Rosylight <[email protected]>

saschanaz · 2025-04-18T07:36:03Z

LGTM

github-actions · 2025-04-18T07:36:21Z

There was an issue merging, maybe try again saschanaz. Details

jakebailey

Thanks, this seems much more obviously correct.

Refactor test script for improved readability and modularity

465e347

format

74fe737

Bashamega marked this pull request as ready for review March 31, 2025 12:24

Bashamega added 4 commits March 31, 2025 14:32

chore: add types

34e146c

-

d04d009

-

c899c09

-

414add9

saschanaz reviewed Apr 1, 2025

View reviewed changes

Switch to function

23e40c5

saschanaz reviewed Apr 1, 2025

View reviewed changes

fix

81e4c8f

Bashamega requested a review from saschanaz April 16, 2025 03:30

saschanaz reviewed Apr 17, 2025

View reviewed changes

Bashamega and others added 2 commits April 18, 2025 08:40

Merge branch 'microsoft:main' into patch-3

4399d1e

-

9e2aba4

-

eaddc8b

saschanaz reviewed Apr 18, 2025

View reviewed changes

src/test.ts Outdated Show resolved Hide resolved

Update src/test.ts

1291a7a

Co-authored-by: Kagami Sascha Rosylight <[email protected]>

jakebailey approved these changes Apr 18, 2025

View reviewed changes

jakebailey merged commit 7304e04 into microsoft:main Apr 18, 2025
5 checks passed

Bashamega deleted the patch-3 branch November 3, 2025 04:18

Refactor test script for improved readability and modularity #1956

Refactor test script for improved readability and modularity #1956

Uh oh!

Conversation

Bashamega commented Mar 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes:

Benefits:

Uh oh!

github-actions bot commented Mar 31, 2025

Uh oh!

saschanaz left a comment

Choose a reason for hiding this comment

Uh oh!

Bashamega commented Apr 1, 2025

Uh oh!

Bashamega commented Apr 1, 2025

Uh oh!

saschanaz Apr 1, 2025

Choose a reason for hiding this comment

Uh oh!

saschanaz Apr 1, 2025

Choose a reason for hiding this comment

Uh oh!

saschanaz Apr 1, 2025

Choose a reason for hiding this comment

Uh oh!

saschanaz Apr 1, 2025

Choose a reason for hiding this comment

Uh oh!

saschanaz Apr 1, 2025

Choose a reason for hiding this comment

Uh oh!

Bashamega commented Apr 1, 2025

Uh oh!

Bashamega commented Apr 3, 2025

Uh oh!

jakebailey commented Apr 17, 2025

Uh oh!

Bashamega commented Apr 17, 2025

Uh oh!

jakebailey commented Apr 17, 2025

Uh oh!

saschanaz commented Apr 17, 2025

Uh oh!

saschanaz Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bashamega commented Apr 18, 2025

Uh oh!

saschanaz commented Apr 18, 2025

Uh oh!

Bashamega commented Apr 18, 2025

Uh oh!

saschanaz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

saschanaz commented Apr 18, 2025

Uh oh!

github-actions bot commented Apr 18, 2025

Uh oh!

jakebailey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Bashamega commented Mar 31, 2025 •

edited

Loading

saschanaz Apr 17, 2025 •

edited

Loading