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

test: add type notation in tests #548

Merged
merged 2 commits into from
Mar 8, 2024
Merged

test: add type notation in tests #548

merged 2 commits into from
Mar 8, 2024

Conversation

Tlacenka
Copy link
Collaborator

@Tlacenka Tlacenka commented Mar 7, 2024

Upon discovering one can use a type argument in toEqual and toStrictEqual, I added these arguments in our tests. They replace the need for satisfies in expected test output.

Note: The type argument cannot be used when the output from expect is a promise that requires a resolves first.

Copy link

nx-cloud bot commented Mar 7, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 5641b0e. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 9 targets

Sent with 💌 from NxCloud.

matejchalk
matejchalk previously approved these changes Mar 7, 2024
packages/utils/src/lib/reports/utils.unit.test.ts Outdated Show resolved Hide resolved
@BioPhoton
Copy link
Collaborator

Could that be a linting rule?

BioPhoton
BioPhoton previously approved these changes Mar 7, 2024
Copy link
Collaborator

@BioPhoton BioPhoton left a comment

Choose a reason for hiding this comment

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

Nice improvmenet!

@Tlacenka Tlacenka dismissed stale reviews from BioPhoton and matejchalk via 5641b0e March 8, 2024 09:42
@Tlacenka Tlacenka requested a review from matejchalk March 8, 2024 09:43
@matejchalk
Copy link
Collaborator

@BioPhoton

Could that be a linting rule?

In general, lots of things could be linting rules. The more practical question is: Are there any linting rules available in the ecosystem? If not, we would have to implement that lint rule ourselves, in which case the cost-benefit ratio becomes highly questionable.

The current @code-pushup/eslint-config contains all the available rules I could find which I thought were beneficial. It's possible I missed something, of course. But mostly I would say that if it hasn't been added already, there's good chance it's because it doesn't exist or I didn't think it was beneficial (an inaccurate linting rule is worse than no linting rule, IMO).

Also, keep in mind that lint rules should be something that's universally a good thing. When it comes to practices that are good in some cases but in other's it's pointless or even counter-productive, then it's very difficult to automate accurately with a lint rule.

Regarding your specific requests:

  • a rule to enforce .toEqual<T>(...) instead of untyped .toEqual(...):
    • Is it available? - Well, it would have to be specific to Vitest, so the place to look is eslint-plugin-vitest. I don't see any such rule there (I don't think any of their rules are TS-specific, actually).
    • Is it universal? - I'm not sure. The typed toEqual is mostly beneficial because of autocomplete, but the correctness checking is already take care of by the runtime assertion.
  • a rule to enforce const x: T = ... instead of const x = ... satisfies T:
    • Is it available? - Since it's a TypeScript feature, the place to look is typescript-eslint. I can't see any rules regarding satisfies usage at all (could be because it's still a fairly new feature 🤷‍♂️).
    • Is it universal? - Potentially. The reason satisfies exists is to be able to do const x: T1 = ... satisfies T2, but I can't think of a practical use case for needing to do const x = ... satisfies T.

So in both cases, we would have to implement our own lint rule. I don't think either are important enough to prioritize such work though.

@Tlacenka Tlacenka merged commit 09a7ab0 into main Mar 8, 2024
19 checks passed
@Tlacenka Tlacenka deleted the test-equals-notation branch March 8, 2024 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants