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(cli-repl): use strict TS flag for test files #2149

Merged
merged 1 commit into from
Sep 4, 2024

Conversation

addaleax
Copy link
Contributor

No description provided.

@@ -884,7 +885,7 @@ describe('CliRepl', function () {
try {
await cliRepl.start('', {});
expect.fail('missed exception');
} catch (err) {
} catch (err: any) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are going to use strict (it doesn't need to happen on this PR) I would prefer to use a bit more strict typing here. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Errors have to be unknown or any because programming error (like trying to use some property of undefined) can also get there so you can never know what kind of error it would be. There's an argument for starting with unknown, though. But in tests any is really probably fine..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we are going to use strict (it doesn't need to happen on this PR)

Well, that's what this PR currently does 🙂

I would prefer to use a bit more strict typing here. WDYT?

Could you give an example of what you have in mind here? Are you talking about this specific line or the PR in general? For catches, TS essentially only gives us a choice between unknown and any – we can use unknown, but it's honestly not super clear to me what the benefit of that by itself is in a test file.

If we want to ensure that we get an Error object here, we should be asserting that using chai regardless of the TS typings we're using, right? And I don't disagree with that, but that would apply to almost all catch {} blocks in tests in our codebases 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Errors have to be unknown or any because programming error (like trying to use some property of undefined) can also get there so you can never know what kind of error it would be

It's the simple fact that you can throw any value, not just any error, in particular things like throw null; work just fine in JS 🙃

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah sorry, I meant that using strict and be more strict on typings.

I know that TypeScript has a limitation on specifying types on catch and I know the excuse from Microsoft 🧌, but probably we are expecting a really specific type of exception here so we don't want to pass the test if the error is a NPE or something else. So I would add a chai assertion making sure that the type is the one we want and fail otherwise (we were expecting exception 'A' but got 'B').

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I would add a chai assertion making sure that the type is the one we want and fail otherwise (we were expecting exception 'A' but got 'B').

I'll put that on our tech alignment sync agenda, because this seems like a gigantic undertaking if we want to start doing it consistently across all our tests 🙂

@addaleax addaleax merged commit 5682192 into main Sep 4, 2024
68 of 71 checks passed
@addaleax addaleax deleted the cli-repl-strict-testing branch September 4, 2024 10:22
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