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

Convert unit tests to Typescript #258

Merged
merged 3 commits into from
May 27, 2024
Merged

Conversation

kravets-levko
Copy link
Collaborator

@kravets-levko kravets-levko commented May 20, 2024

Part of PECO-1390

Initially I planned to convert unit tests gradually, but there were issues with collecting coverage data. So I had to convert everything at once

export default class DBSQLClient extends EventEmitter implements IDBSQLClient, IClientContext {
private static defaultLogger?: IDBSQLLogger;

private readonly config: ClientConfig;

private connectionProvider?: IConnectionProvider;
protected connectionProvider?: IConnectionProvider;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally we test through the public interface. Do we need to expose these to get reasonable test coverage? I don't know the standards in the Node world, but in general my approach is, if there are methods that should be private, but I need to expose in order to test, I make a new type where those methods make sense as public methods, and have the original type call the new type to accomplish the work. This allows you to mock the new class in the tests of the original class, and not feel compelled to have a large public interface that consumers really shouldn't call. Given how big a change we are already doing, I won't block this PR on that, but it's something we should consider before we make another release of this library.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I took another look at these changes and totally agree with you. I'll revert all similar changes (private -> protected) and temporarily just suppress TS errors with @ts-expect-error

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done 275bdbf

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For now I kept code that uses private fields and methods - this PR is already too big to add more refactoring. However, it turns out that TS allows to access private and protected fields using string indexing (obj['prop']) - TS still checks if property exists and keeps things type-safe, so I think it's still better than untyped code or with errors suppressed

Copy link
Contributor

@benc-db benc-db left a comment

Choose a reason for hiding this comment

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

I'm approving but given how big an interface change we have here, primarily to remove type errors from tests, please get another reviewer before merge and consider my comment about refactoring in a way that introduces new types, rather than modifying the interface of existing types.

@kravets-levko kravets-levko marked this pull request as draft May 20, 2024 21:23
Signed-off-by: Levko Kravets <[email protected]>
@databricks databricks deleted a comment from codecov-commenter May 21, 2024
@kravets-levko kravets-levko marked this pull request as ready for review May 21, 2024 10:58
Signed-off-by: Levko Kravets <[email protected]>
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 93.54839% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 92.93%. Comparing base (3eed509) to head (6d2718b).

Files Patch % Lines
...nnection/auth/DatabricksOAuth/AuthorizationCode.ts 89.28% 2 Missing and 1 partial ⚠️
lib/connection/auth/DatabricksOAuth/index.ts 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #258      +/-   ##
==========================================
- Coverage   93.06%   92.93%   -0.13%     
==========================================
  Files          65       64       -1     
  Lines        1586     1572      -14     
  Branches      280      282       +2     
==========================================
- Hits         1476     1461      -15     
+ Misses         46       44       -2     
- Partials       64       67       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kravets-levko kravets-levko merged commit 3c29fe2 into main May 27, 2024
8 checks passed
@kravets-levko kravets-levko deleted the convert-unit-to-typescript branch May 27, 2024 15:37
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