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

Improve tests with expected types and ensuring every line generates code #42

Open
TheSpyder opened this issue Nov 2, 2021 · 4 comments
Labels
help wanted Extra attention is needed
Projects

Comments

@TheSpyder
Copy link
Owner

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 TheSpyder added the help wanted Extra attention is needed label Nov 2, 2021
@TheSpyder TheSpyder added this to To do in 1.0 release via automation Nov 2, 2021
@spocke
Copy link
Collaborator

spocke commented Dec 2, 2021

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
Copy link
Collaborator

spocke commented Dec 2, 2021

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
Copy link
Collaborator

spocke commented Dec 4, 2021

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

@TheSpyder TheSpyder moved this from To do to In progress in 1.0 release Jan 25, 2022
@TheSpyder
Copy link
Owner Author

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 TheSpyder removed this from In progress in 1.0 release Aug 26, 2022
@TheSpyder TheSpyder added this to Todo in 2.0 Release via automation Aug 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
Development

No branches or pull requests

2 participants