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: add tsconfig for tests/ directory #425

Merged
merged 1 commit into from
Dec 20, 2023
Merged

Conversation

achou11
Copy link
Member

@achou11 achou11 commented Dec 20, 2023

Closes #416

This doesn't include any npm script updates or a step to typecheck the tests. Mostly useful as we're writing tests for editor feedback for now.

Didn't feel inclined to include fixes to the type issues that were found as a result of this PR. Happy to try since I believe most are probably straightforward to address.

Current state of checks as of now:

Found 173 errors in 16 files.

Errors  Files
    14  tests/bitfield-rle.js:7
     4  tests/blob-store/blob-store.js:416
     5  tests/blob-store/combine-states.js:40
    12  tests/blob-store/live-download.js:19
    53  tests/core-manager.js:25
     1  tests/core-ownership.js:79
     4  tests/data-type.js:123
     4  tests/datastore.js:29
    23  tests/discovery/dns-sd.js:19
     8  tests/discovery/local-discovery.js:69
     4  tests/fastify-plugins/blobs.js:295
     7  tests/helpers/replication-state.js:3
    13  tests/invite-api.js:33
     6  tests/local-peers.js:220
     9  tests/sync/core-sync-state.js:328
     6  tests/sync/namespace-sync-state.js:111

@achou11 achou11 requested a review from gmaclennan December 20, 2023 19:08
Copy link
Member

@gmaclennan gmaclennan left a 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. I'd be inclined to just put // @ts-nocheck at the top of these existing files, I don't think we gain anything from jumping through hoops to make typescript happy, and it's possible that fixing some of the type issues could reduce test coverage. I can see the utility when writing new test files, and then electing to ignore errors and then add // @ts-nocheck when done.

@achou11
Copy link
Member Author

achou11 commented Dec 20, 2023

I'd be inclined to just put // @ts-nocheck at the top of these existing files

Do you prefer I include that as part of this PR? Personally I'd even be fine with not doing that as long as we don't have plans to include a typechecking step for these files. I'm mostly interested in just making it easier to write the tests with the TS-powered assistance

EDIT: I think i'd actually prefer to not add the annotation at all because I still want want kind of visual feedback about potential errors that are actually type errors. i guess in theory the tests ideally break in that case but there are definitely times where passing in the wrong types for arguments is happening and not clear because of the lack of feedback for me 😄

@achou11 achou11 merged commit a209b4f into main Dec 20, 2023
7 checks passed
@achou11 achou11 deleted the 416/tests-tsconfig branch December 20, 2023 19:53
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.

type checking in tests/ directory doesn't use tsconfig (fully?)
2 participants