-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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: TypeScript type specification strength tests #32905
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
82 changes: 82 additions & 0 deletions
82
x-pack/plugins/canvas/public/lib/aeroelastic/__fixtures__/typescript/typespec_tests.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,82 @@ | ||
import { select } from '../../select'; | ||
import { Json, Selector } from '../..'; | ||
|
||
/* | ||
|
||
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. | ||
|
||
*/ | ||
|
||
((): void => { | ||
/** | ||
* TYPE TEST SUITE | ||
*/ | ||
|
||
(function jsonTests(plain: Json): void { | ||
// numbers are OK | ||
plain = 1; | ||
plain = NaN; | ||
plain = Infinity; | ||
plain = -Infinity; | ||
plain = Math.pow(2, 6); | ||
// other JSON primitive types are OK | ||
plain = false; | ||
plain = 'hello'; | ||
plain = null; | ||
// structures made of above and of structures are OK | ||
plain = {}; | ||
plain = []; | ||
plain = { a: 1 }; | ||
plain = [0, null, false, NaN, 3.14, 'one more']; | ||
plain = { a: { b: 5, c: { d: [1, 'a', -Infinity, null], e: -1 }, f: 'b' }, g: false }; | ||
|
||
// typings:expect-error | ||
plain = undefined; // it's undefined | ||
// typings:expect-error | ||
plain = a => a; // it's a function | ||
// typings:expect-error | ||
plain = [new Date()]; // it's a time | ||
// typings:expect-error | ||
plain = { a: Symbol('haha') }; // symbol isn't permitted either | ||
// typings:expect-error | ||
plain = window || void 0; | ||
// typings:expect-error | ||
plain = { a: { b: 5, c: { d: [1, 'a', undefined, null] } } }; // going deep into the structure | ||
|
||
return; // jsonTests | ||
})(null); | ||
|
||
(function selectTests(selector: Selector): void { | ||
selector = select((a: Json) => a); // one arg | ||
selector = select((a: Json, b: Json): Json => `${a} and ${b}`); // more args | ||
selector = select(() => 1); // zero arg | ||
selector = select((...args: Json[]) => args); // variadic | ||
|
||
// typings:expect-error | ||
selector = (a: Json) => a; // not a selector | ||
// typings:expect-error | ||
selector = select(() => {}); // should yield a JSON value, but it returns void | ||
// typings:expect-error | ||
selector = select((x: Json) => ({ a: x, b: undefined })); // should return a Json | ||
|
||
return; // selectTests | ||
})(select((a: Json) => a)); | ||
|
||
return; // test suite | ||
})(); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
21 changes: 21 additions & 0 deletions
21
x-pack/plugins/canvas/public/lib/aeroelastic/tsconfig.json
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
{ | ||
"extends": "../../../../../tsconfig", | ||
"compilerOptions": { | ||
"module": "commonjs", | ||
"lib": ["esnext", "dom"], | ||
"noImplicitAny": true, | ||
"strictNullChecks": true, | ||
"strictFunctionTypes": false, | ||
"strictPropertyInitialization": true, | ||
"noImplicitThis": true, | ||
"noImplicitReturns": true, | ||
"forceConsistentCasingInFileNames": true, | ||
"noEmit": true, | ||
"baseUrl": ".", | ||
"paths": { | ||
"layout/*": ["aeroelastic/*"] | ||
}, | ||
"types": ["@kbn/x-pack/plugins/canvas/public/lib/aeroelastic"] | ||
}, | ||
"exclude": ["node_modules", "**/*.spec.ts", "node_modules/@types/mocha"] | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6675,7 +6675,7 @@ [email protected]: | |
resolved "https://registry.yarnpkg.com/commander/-/commander-0.6.1.tgz#fa68a14f6a945d54dbbe50d8cdb3320e9e3b1a06" | ||
integrity sha1-+mihT2qUXVTbvlDYzbMyDp47GgY= | ||
|
||
commander@2, [email protected], commander@^2.11.0, commander@^2.12.1, commander@^2.19.0, commander@^2.9.0: | ||
commander@2, [email protected], commander@^2.11.0, commander@^2.12.1, commander@^2.12.2, commander@^2.19.0, commander@^2.9.0: | ||
version "2.19.0" | ||
resolved "https://registry.yarnpkg.com/commander/-/commander-2.19.0.tgz#f6198aa84e5b83c46054b94ddedbfed5ee9ff12a" | ||
integrity sha512-6tvAOO+D6OENvRAh524Dh9jcfKTYDQAqvqezbCW82xj5X0pSrcpxtvRKHLG0yBY6SD7PSDrJaj+0AiOcKVd1Xg== | ||
|
@@ -23530,12 +23530,14 @@ typescript@^3.3.3333: | |
resolved "https://registry.yarnpkg.com/typescript/-/typescript-3.3.3333.tgz#171b2c5af66c59e9431199117a3bcadc66fdcfd6" | ||
integrity sha512-JjSKsAfuHBE/fB2oZ8NxtRTk5iGcg6hkYXMnZ3Wc+b2RSqejEqTaem11mHASMnFilHrax3sLK0GDzcJrekZYLw== | ||
|
||
ua-parser-js@^0.7.18: | ||
version "0.7.18" | ||
resolved "https://registry.yarnpkg.com/ua-parser-js/-/ua-parser-js-0.7.18.tgz#a7bfd92f56edfb117083b69e31d2aa8882d4b1ed" | ||
integrity sha512-LtzwHlVHwFGTptfNSgezHp7WUlwiqb0gA9AALRbKaERfxwJoiX0A73QbTToxteIAuIaFshhgIZfqK8s7clqgnA== | ||
typings-tester@^0.3.2: | ||
version "0.3.2" | ||
resolved "https://registry.yarnpkg.com/typings-tester/-/typings-tester-0.3.2.tgz#04cc499d15ab1d8b2d14dd48415a13d01333bc5b" | ||
integrity sha512-HjGoAM2UoGhmSKKy23TYEKkxlphdJFdix5VvqWFLzH1BJVnnwG38tpC6SXPgqhfFGfHY77RlN1K8ts0dbWBQ7A== | ||
dependencies: | ||
commander "^2.12.2" | ||
|
||
ua-parser-js@^0.7.9: | ||
ua-parser-js@^0.7.18, ua-parser-js@^0.7.9: | ||
version "0.7.19" | ||
resolved "https://registry.yarnpkg.com/ua-parser-js/-/ua-parser-js-0.7.19.tgz#94151be4c0a7fb1d001af7022fdaca4642659e4b" | ||
integrity sha512-T3PVJ6uz8i0HzPxOF9SWzWAlfN/DavlpQqepn22xgve/5QecC+XMCAtmUNnY7C9StehaV6exjUCI801lOI7QlQ== | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 😄There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... 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):void
is gone, so it moves cruft from here to there)let
, as there's no initial value, it won't actually adhere to the type unless the type includes theundefined
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 wasundefined
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All great points, I'm open to refining the approach in subsequent PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @spalger, linked your suggestions in the PR description so we see them all in one place, now or post-merge.