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: convert tests to jest #929

Merged
merged 2 commits into from
Jul 16, 2020
Merged

test: convert tests to jest #929

merged 2 commits into from
Jul 16, 2020

Conversation

djskinner
Copy link
Contributor

Convert the following packages' test files to TypeScript and jest:

  • plugin-node-in-project
  • plugin-node-device
  • plugin-node-surrounding-code
  • plugin-node-uncaught-exception

@bugsnagbot
Copy link
Collaborator

bugsnagbot commented Jul 13, 2020

@bugsnag/browser bundle size diff

Minified Minfied + Gzipped
Before 39.56 kB 12.22 kB
After 39.56 kB 12.22 kB
± No change No change

Generated by 🚫 dangerJS against 1abf8f7

@@ -1,38 +1,37 @@
const { describe, it, expect } = global
/* eslint-disable @typescript-eslint/no-non-null-assertion */
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 wonder if it's worth disabling this rule across all test files as its usage is often legitimate and useful.

Often there are cases where a previous assertion asserts the existence of something. Unfortunately the typings of expect aren't sufficient to avail the TS compiler of such knowledge (perhaps it is possible to do with better typings that use assertion signatures - I'm not sure). Even without the benefit of previous assertions, it seems legitimate to me to make assumptions about the existence of things because the test would just fail otherwise.

FWIW we agreed that it would be OK disable this rule in the dashboard-js project though we haven't got round to doing it yet. That might be because in that project we are more relaxed about overriding rules on a per-line or entire file basis where judged appropriate.

On a related note I've seen a few places in the tests where the optional chaining ?. operator would be useful. Though currently jest doesn't like it when used. I think we might need to update our TypeScript/jest/babel dependencies if we want to make use of that one.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's a reasonable thing to assert. Since the implementation is JS, the type system doesn't guarantee that such things are genuinely non-null.

Maybe updating the test dev dependencies should come in via another PR. The ?. and also ! are nice operators for terse TS so it would be good to have access to them.

@bengourley bengourley merged commit fd9523d into next Jul 16, 2020
@bengourley bengourley deleted the convert-tests-to-jest branch July 16, 2020 16:22
@djskinner djskinner mentioned this pull request Jul 22, 2020
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