Skip to content
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: Convert lib/tests to TypeScript #1345

Merged
merged 10 commits into from
Jan 21, 2025
Merged

chore: Convert lib/tests to TypeScript #1345

merged 10 commits into from
Jan 21, 2025

Conversation

kemmerle
Copy link
Contributor

@kemmerle kemmerle commented Jan 16, 2025

Description and Context

In this PR, I've converted the following files from /lib/__tests__ to TypeScript:

commonOpts.test.ts
dependencyManagement.test.ts
projectsLogManager.test.ts
projects.test.ts

serverlessLogs.test.ts is the final file from /lib/__tests__ that needs TS conversion, but the prerequisite files haven't been converted yet. I plan to make that its own PR.

TODO

  • Fix failing tests
  • Address feedback

Who to Notify

@brandenrodgers @camden11 @joe-yeager

@kemmerle kemmerle marked this pull request as draft January 16, 2025 19:58
@kemmerle kemmerle marked this pull request as ready for review January 17, 2025 15:16
Copy link
Contributor

@camden11 camden11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, this is looking good! Just a few minor comments

lib/__tests__/commonOpts.test.ts Outdated Show resolved Hide resolved
lib/__tests__/dependencyManagement.test.ts Outdated Show resolved Hide resolved
lib/commonOpts.ts Outdated Show resolved Hide resolved
APP_FUNCTION: 'APP_FUNCTION',
AUTOMATION_ACTION: 'AUTOMATION_ACTION',
REACT_EXTENSION: 'REACT_EXTENSION',
} as const;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just grab these from LDL

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was getting an error when I attempted that--it didn't look to me like the enums folder where this is located had an export assigned in LDL's package.json. Am I missing anything?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh interesting, I didn't realize those aren't exported. That's fine then!

camden11
camden11 previously approved these changes Jan 21, 2025
Copy link
Contributor

@camden11 camden11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

APP_FUNCTION: 'APP_FUNCTION',
AUTOMATION_ACTION: 'AUTOMATION_ACTION',
REACT_EXTENSION: 'REACT_EXTENSION',
} as const;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh interesting, I didn't realize those aren't exported. That's fine then!

@kemmerle kemmerle merged commit f4d24e4 into main Jan 21, 2025
1 check passed
@kemmerle kemmerle deleted the convertts-1 branch January 21, 2025 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants