rescript-webapi icon indicating copy to clipboard operation
rescript-webapi copied to clipboard

Improve tests with expected types and ensuring every line generates code

Open TheSpyder opened this issue 3 years ago • 4 comments

There are two problems with our tests:

  1. They use let _ = blah() which means if the type of a method changes the test still compiles. This is not good.
  2. By always using _ to avoid exporting the reference, some lines are optimised away by ReScript and never emitted (this is arguably a bug, but it's what we are stuck with for now).

To solve the first problem, change things like

let _ = range->getClientRects

to

let _: array<Dom.domRect> = range->getClientRects

For the second, about all we can do is audit the JS and see which lines don't result in compiled output. I have found that externals returning an optional value in a let _ = line are not emitted, here's how I solved that: https://github.com/tinymce/rescript-webapi/blob/b138ae2982d24193473e01929251871ea2d4ba65/tests/Webapi/Dom/Webapi__Dom__Document__test.res#L64-L67

The more I think about this the more I believe it should be logged as a bug with ReScript. But we should find them all first to make the bug report more valuable.

TheSpyder avatar Nov 02 '21 04:11 TheSpyder

I made them export a reference in a PR so I changed let _ = range->getClientRects to let rects = range->getClientRects that way the compiler will not throw them away I don't mind it adding a export to the test module no one will include that directly anyway we just make sure they are not shipped in the npm package.

So maybe we switch it to this: let clientRects: array<Dom.domRect> = range->getClientRects ?

spocke avatar Dec 02 '21 14:12 spocke

I updated a PR I made to use that approach I think it's short and easy to read. https://github.com/tinymce/rescript-webapi/pull/48/files

It adds a bit of code to the generated js files for the exports but not having to boilerplate code for every property/function we test would be nice.

spocke avatar Dec 02 '21 14:12 spocke

Noticed a different variant with let test_foo_bar =.. not sure I like snake case for rescript things.

spocke avatar Dec 04 '21 08:12 spocke

I have started a branch for this, there's a lot to do, but I've nearly covered the 10 files in the top-level webapi folder. I'm not looking forward to updating the dom folder 😂

TheSpyder avatar Feb 02 '22 04:02 TheSpyder