Test: TypeScript type specification strength tests

[monfera]

Summary

I'm proposing this PR for

• merging after a review ensuring it's not breaking something central (I'd like these types to be tested and plan to add more such test cases)
• discussion and future iteration as we might want to broaden the addition of static type strength tests

Add type strength lock-in

Note: what's important here is the purpose:

1. get an error if a code change accidentally

• dilutes some type strength that's locked in via failing static type test cases
• overconstrains type strength that's locked in via type test cases that must pass

2. also, as with runtime unit tests, the cases act as documentation of the types via examples

Ie. PRs against this branch are welcome, as long as they still make it convenient and performant to rein in type strengths. Currently, typings-tester is used, and the approach had been shaped with @restrry @spalger and @joshdover

See the static type check file typespec_tests.ts as an example.

Motivation for adding type strength lock-in

From the code comment:

Type checking isn't too useful if future commits can accidentally weaken the type constraints, because a TypeScript linter will not complain - everything that passed before will continue to pass. The coder will not have feedback that the original intent with the typing got compromised. To declare the intent via passing and failing type checks, test cases are needed, some of which designed to expect a TS pass, some of them to expect a TS complaint. It documents intent for peers too, as type specs are a tough read.

Run compile-time type specification tests in the kibana root with:

yarn typespec

Test "cases" expecting to pass TS checks are not annotated, while ones we want TS to complain about are prepended with the comment

// typings:expect-error

The test "suite" and "cases" are wrapped in IIFEs to prevent linters from complaining about the unused binding. It can be structured internally as desired.

Note: it's also possible for accidental changes to overconstrain the type systems, and the test cases also serve as examples for the types, so tests expecting not to fail are also included.

Alternatives considered

• dtslint looks great when the purpose is to lock in exposed types of a library API but couldn't quickly figure it out for such local use when the goal is to lock in type strengths in an arbitrary implementation corner of an application
• Another proposal was to use runtime tests which in fact would kick off small command line scripts for running tslint, expecting pass or failure as appropriate, but that's more complicated; way less performant and less informative (either a binary pass/fail, or otherwise one would need to test for specific tslint error messages which may change over time)

As a consequence, typings-tester was picked as @restrry pointed to their use of it in redux-saga eg. here

Suggestions and improvement opportunities

In general, the PR's focus here is of a test writer: when adding types, I'd like to ensure that they retain their original intent going forward, and any future type creep is detected. This current solution already meets this basic goal, but would benefit from generalization so others can lock in type strengths, and they be part of the CI test suite; glad to review any PR to this branch or further iteration after merging this.

From @w33ble: could we not duplicate tsconfig.json? A: yes it might possible but I don't know yet; also, different parts of the application may use different flags and other props in compilerOptions. Eg. an initial TS conversion may specify fairly loose checks, and a later tightening iteration may activate more flags. Similarly, what's included/excluded etc may need to be specific.

From @joshdover: Maybe we should create a tsconfig.typetests.json that only includes a certain pattern for these tests…something like **/*.typetest.ts
Yes, agreed, as long as everyone's happy with this convention. Ie. we could have a centralized tsconfig.json that still piggybacks on x-pack/tsconfig.json but adds config items specific for running these kinds of static type checks in broader areas of the code.

From me (quite obvious but unsure of the execution): though there's now a jury-rigged yarn typespec CLI call that can run the type test, it's not currently part of the overall Kibana test locally or in CI. I'd appreciate if someone could add a PR to the branch or just a commit, as without this, accidental watering-up of tests can still occur, developers shouldn't remember to run this separate thing. Also, I suppose, this test command should still be available separately for very quick runs during development, preferably taking a directory argument so a specific part of the implementation can be tested without having to run it on everything.

From @spalger: use of module.typespec.ts; lowering typing noise related to the current IIFEs

Misc change: improvement for some types related to the layout engine

• much more specific types (we should eventually reuse the Json ones)
• better type names

Misc change: other minor items

• simplification of select
• renaming of a couple of files

[elasticmachine]

Pinging @elastic/kibana-canvas

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[spalger]

What if we used module.typespec.ts and told tslint to ignore .typespec.ts files? Could we avoid the IIFE gymnastics and make the variable declarations a little more straightforward?

let plain: Json;
// numbers are OK
plain = 1;
plain = NaN;
plain = Infinity;
plain = -Infinity;
plain = Math.pow(2, 6);

[monfera]

@spalger thanks for the comments!

Re module.typespec.ts I'm not familiar with it, a one-liner PR against this branch or some other pointer would be welcome, or in theory, this conversion could be done as a subsequent PR by your or someone who has the chops.

Re IIFE: my purpose was to 1) make it a bit like describe / it; 2) use informative function names for some structure without resorting to comment lines, 3) limit the scope of these variables so the reader can see what's what; 4) allow the movement of tested units (now IIFEs) with structured editing as done in Lisp code (whereas if it's line based, the logical boundaries are unclear so it's super error prone to change the sequence of anything or rename something etc.).

I could solve these with either IIFEs (or analogous) or by using separate files, but I didn't want to make files too small - it's nice to cover a sensible unit in one file (ymmv though). Also, maybe someone spends the time and defines describe / it functions for making it look even more like unit tests, well assuming it's a goal at all 😄

[monfera]

... taking another look, I agree it'd be less tedious if I used eg. let plain: Json etc. instead of passing these as typed IIFE args, which indeed complexifies like the WaPo 😸 This is why I stuck with it (welcome to try and suggest alternatives):

• linter will complain that I bind values but don't actually use the value (I could return the value but then the simplicity of returning void is gone, so it moves cruft from here to there)
• with the non-value-binding let, as there's no initial value, it won't actually adhere to the type unless the type includes the undefined value (I'm not sure if it's a tslint issue or if I just found it weird to have a purportedly type X variable which then was undefined which is not allowed by the typespec)

One more thing I planned with the functions (which are immediately invoked right now, but don't have to be): potentially reuse them. Eg. pass on various parameters to try things in different combinations.

[spalger]

All great points, I'm open to refining the approach in subsequent PRs.

[monfera]

Thanks @spalger, linked your suggestions in the PR description so we see them all in one place, now or post-merge.

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[spalger]

LGTM

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request