mathjs icon indicating copy to clipboard operation
mathjs copied to clipboard

Writing tests in typescript

Open ChristopherChudzicki opened this issue 2 years ago • 6 comments

In https://github.com/josdejong/mathjs/pull/2544#issuecomment-1109396849 @gwhitney pointed out that index.d.ts and index.ts are becoming rather large. Setting index.d.ts aside (admittedly, the problem is worse in that file!), the comment got me thinking... maybe it would be beneficial to just convert (gradually) the tests/ directory to typescript. Some benefits:

  • tests for typings would be colocated with tests for "math logic"... probably get better / more accurate coverage of typings
  • naturally breaks apart the big index.ts file (doesn't help with index.d.ts)
  • being able to write new tests in ts would be nice!
  • I think this would be one way to achieve the goal of https://github.com/josdejong/mathjs/issues/2472:
    • if the expectation were that tests for new contributions be written in ts, they'd have to have types
    • converting old tests would (gradually) guarantee the same for existing APIs

Would need to:

  • set something up to compile ts tests to to js for running (running with ts-node is another possibility)
  • extending linting to tests/*.ts... That should be straightforward after #2544

ChristopherChudzicki avatar Apr 26 '22 22:04 ChristopherChudzicki

I have no objection to this in principle, but presumably we would want some standardized test harness to run the TypeScript tests rather than rolling our own. I presume there is no such thing as mocha-ts or the like? Presuming not, is there a de facto standard typescript test harness and how bad would the switchover be, or would we just run both harnesses and gradually move individual files from one to the other as they were touched?

If we went this path would we be essentially obliged to eliminate the uses of 'any' in the current type declarations for clean testing, and if so, is there much hope for removing those 'any's?

As always, my comments/thoughts on TypeScript matters are relatively uninformed by real-world TypeScript experience.

gwhitney avatar Apr 26 '22 22:04 gwhitney

We wouldn't use a separate test harness, we'd still use mocha. I think there are basically two possible approaches: (1) compile the ts to js, be sure to include sourcemaps, watch for changes, run mocha via its normal CLI / nodeJS on the generated code. Or (2) use ts-node like

mocha --require ts-node/register --extensions ts,tsx --watch --watch-files src 'tests/**/*.{ts,tsx}' [...args]

(2) seems like a good option for MathJS. [Aside: I don't think (1) would be that hard, but why bother. It'd be different if src/ was written in typescript and we had to compile to JS anyway. Then we'd have to have all that infrastructure in place.]

If we went this path would we be essentially obliged to eliminate the uses of 'any' in the current type declarations for clean testing, and if so, is there much hope for removing those 'any's?

No... TS will basically let you do anything with an explicit any. We'd continue to get little type assurance from them, but that's already true.

We would get new errors if the test file imports things that don't have type declarations... but then we'd type them!

ChristopherChudzicki avatar Apr 26 '22 23:04 ChristopherChudzicki

Or (2) use ts-node like

Note that for running index.ts as we currently do (no mocha) we couldn't get ts-node to work, but rather ended up with node and some funky command-line options that you can see in the package.json file.

TS will basically let you do anything with an explicit any.

Well, but what I really meant is that I for one would not be very comfortable with any warnings in mathjs's main suite of unit tests, although ultimately of course that's for @josdejong to call. So I'd then strongly recommend some scheme to ensure there were no warnings in the bulk of unit tests, and I was worried that might entail purging all of the 'any's.

gwhitney avatar Apr 27 '22 00:04 gwhitney

It makes sense to me @ChristopherChudzicki , thanks for starting this discussion. I see two challenges:

  • The typescript index files become too large
  • When implementing a new function, there are now soo many places where you have to do something to achieve that: write the function itself, write tests for the function, write embedded docs, write type definitions, write tests for the type definitions. I think it may help to have the type definitions of a function alongside the function itself, and the same for the tests.

I'm not sure though if this currently can work out nicely, so an experiment to try it out would be great.

Shall we keep the any discussion focused here: https://github.com/josdejong/mathjs/pull/2544#discussion_r858219207?

josdejong avatar Apr 28 '22 10:04 josdejong

An experiment definitely seems like a good idea idea. Re

When implementing a new function, there are now soo many places where you have to do something to achieve that ... write tests for the type definitions.

One reason I think this could be worthwhile is that I think it would actually reduce the number of things you have to do to add a function. In particular, I think separate tests for the types and the runtime behavior would no longer be necessary. If the runtime functionality is tested in typescript, then we're also testing the type declarations for that behavior. At least I suspect this is mostly true. There may be some cases where expectTypeOf is still desirable.

ChristopherChudzicki avatar Apr 28 '22 11:04 ChristopherChudzicki

@ChristopherChudzicki Potentially relevant discussion: https://github.com/josdejong/mathjs/pull/2537. I was considering breaking index.ts and index.d.ts apart as part of that PR but decided to hold off. Maybe we can collaborate after it's merged.

mattvague avatar May 20 '22 03:05 mattvague