-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
chore(js-ts): Convert app/util/test/utils.js to TypeScript #11279
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
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.
LGTM
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 like unit tests are failing
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.
Seems that there are other areas where this file is still being imported as utils.js
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.
LGTM
Edit: nevermind, as mentioned some unit tests are failing
Quality Gate failedFailed conditions |
@kylanhurt please check this pr. I guess now this can be merged as the build is working fine except for sonar, and that is due to the existing TODO comments. |
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 to me.
interface TestConfig { | ||
fixtureServerPort?: number; | ||
} | ||
|
||
export const testConfig: TestConfig = {}; |
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.
Is this really an optional prop? I checked the code base and I can't find fixtureServerPort
used anywhere, yet in the function below we reference testConfig.fixtureServerPort ?? 12345
. It seems like this will always be 12345
regardless. testConfig
doesn't even have a mocked key value pair?
testConfig: {
fixtureServerPort: 12345
}
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.
@Daniel-Cross We made it optional because of the coalescing operator. Our reasoning was that the below line does have a fallback mechanism if the port is not defined.
testConfig.fixtureServerPort ?? FIXTURE_SERVER_PORT;
Also, usage is there in shim.js
. If the launch arguments contain a fixtureServerPort, it's assigned to testConfig.fixtureServerPort. Else, the fallback value is FIXTURE_SERVER_PORT
if (isTest) {
const raw = LaunchArguments.value();
testConfig.fixtureServerPort = raw?.fixtureServerPort
? raw.fixtureServerPort
: FIXTURE_SERVER_PORT;
}
This PR has been automatically marked as stale because it has not had recent activity in the last 90 days. It will be closed in 7 days. Thank you for your contributions. |
This PR was closed because there has been no follow up activity in 7 days. Thank you for your contributions. |
Description
This PR converts the
app/util/test/utils.js
file to TypeScript (utils.ts
). The conversion process involved adding type annotations, creating interfaces, and cleaning up unnecessary comments.Motivation and Context
As part of our ongoing efforts to migrate the codebase to TypeScript, we are converting JavaScript files to TypeScript. This conversion enhances type safety, improves code maintainability, and provides better developer experience through improved tooling support.
Changes
utils.js
toutils.ts
TestConfig
interface for better type checkingnotes.md
file used for conversion planningType of change
Checklist
Screenshots (if appropriate)
N/A
Additional context
This PR is part of the ongoing TypeScript migration effort. No functional changes were made to the code logic.
Devin Information
If you have any feedback, you can leave comments in the PR and I'll address them in the app!