feat: added `tests/typescript` to test supported TS examples #6775

btw17 · 2023-01-13T23:30:06Z

The basics

I branched from develop
My pull request is against develop
My code follows the style guide
I ran npm run format and npm run lint

The details

Resolves

This adds support for testing theoretical Blockly use cases, as mentioned in #6639 (review)

Proposed Changes

Updated npm run test to include a new suite of tests connected to tests/typescript:

The test passes if everything there compiles.
The test fails if the files fail to compile.
- The error output from the tsc command displays in the console output.

Behavior Before Change

One less test suite to run.

Behavior After Change

One more test suite to run.

Reason for Changes

To enable testing fake custom fields. This allows us to see how core updates impact use cases we'd like to support.

Test Coverage

N/A

Documentation

I don't believe so.

Additional Information

I added one example test case that's related to a discussion in fix: replace 'AnyDuringMigration' for core/field.ts value functions #6639 about allowing Field subclasses to have different type information for what's passed into setValue than is stored on the Field.
I've tested the console output with npm run test, verifying it passes when the command passes, fails when the command fails, and properly outputs the log info needed to debug the failure.

cpcallen

Yay for more tests!

cpcallen · 2023-01-17T16:25:37Z

.eslintignore

@@ -11,6 +11,7 @@
 /tests/screenshot/*
 /tests/test_runner.js
 /tests/workspace_svg/*
+/tests/typescript/*


How much work would it be to get the new tests to lint cleanly?

I'm not sure how straightforward it will be, though I'll look further into it this upcoming Friday. And get back to you when I've looked into it a bit more.

The configuration in .eslintrc.json points to the root ./tsconfig.json via parserOptions which I believe was the main issue, though there may be around that. These tests need their own tsconfig so they aren't built and packaged with the rest of the code.

Updated! The tsconfig is defined in an array, which I hadn't noticed at my quick glance earlier, so I just had to add the second override.

Of note, I updated .eslintrc.json to .eslintrc.js since we likely want the same settings for the overrides, and it's easier to keep them in sync via JS than in JSON.

tests/typescript/README.md

scripts/gulpfiles/test_tasks.js

tests/typescript/src/field/different_user_input.ts

tests/typescript/tsconfig.json

tests/typescript/src/field/different_user_input.ts

btw17 · 2023-01-20T21:30:43Z

Should be all set! Please check my changes which update .eslintrc.json to .eslintrc.js. Other than restructuring the file for JS, I did the following updates:

Added an ignore pattern for .eslint.js (which was being picked up otherwise and triggered an error due to using module.exports)
Modified the file pattern for the override connected to ./tsconfig.json to just refer to ./core TS files instead of all TS files
Added the override for ./tests/typescript/tsconfig.json

cpcallen

In addition to changing the tsc output directory (see below), can you update .eslintrc.js to follow the JS style guide, please? E.g.:

Indentation by two spaces.
Use single quotes for strings, no quoting of property names that are valid identifieres,
Ends with \n.

You can probably just run clang-format on it. Better: modify the config so that this file is always formatted when npm run format is run.

cpcallen · 2023-02-01T17:30:32Z

scripts/gulpfiles/test_tasks.js

+ */
+function typeDefinitions() {
+  return runTestCommand('type_definitions',
+    'tsc -p ./tests/typescript/tsconfig.json');


Please write output to somewhere in build/ rather than in tests/.

I think it would be preferable to do so using -outDir and path.join(BUILD_DIR, 'tests' /* optionally more */) as buildJavaScript (in build_tasks.js) does.

cpcallen · 2023-02-01T17:31:05Z

.eslintignore

@@ -11,6 +11,7 @@
 /tests/screenshot/*
 /tests/test_runner.js
 /tests/workspace_svg/*
+/tests/typescript/dist/*


This should not be necessary.

When this is not ignored, I get an error like below:

5:1 error Parsing error: 'import' and 'export' may appear only with 'sourceType: module'

Basically it's trying to run lint on the JS files that are created in /tests/typescript/dist

.eslintrc.js

cpcallen · 2023-02-03T19:11:09Z

.eslintrc.js

+  'extends' :
+            [
+              'eslint:recommended',
+              'google',
+            ],


Go home formatter, you're drunk:

Suggested change

'extends' :

[

'eslint:recommended',

'google',

],

extends: [

'eslint:recommended',

'google',

],

(and similarly for overrides.)

cpcallen · 2023-02-03T19:11:50Z

.eslintrc.js

@@ -0,0 +1,215 @@
+const rules = {
+  'curly': ['error'],


Single quotes not required for property names that are valid identifiers:

Suggested change

'curly': ['error'],

curly: ['error'],

(and many below).

Looks like the linter wants consistently quoted properties. In TypeScript, it's best practice to quote keys you don't know at compile time. For example:

interface Test { knownKey: string; [key: string]: number; } const test: Test = { knownKey: 'value', 'unknownOne': 1, 'unknownTwo': 2 };

With that principle in mind, maybe it's valid to keep the keys that are defined by eslint outside of this file quoted.

In TypeScript, it's best practice to quote keys you don't know at compile time.

TIL. Though I'm not sure that argument is exactly applicable here, since I'd guess that the format of the tsconfig data structure is probably fully defined somewhere in the tsc source code.

scripts/gulpfiles/build_tasks.js

cpcallen · 2023-02-03T19:16:43Z

scripts/gulpfiles/config.js

+// Directory where typescript compiler can be found for `./tests/typescript`
+// files.


This description seems confusing to me. How about:

Suggested change

// Directory where typescript compiler can be found for `./tests/typescript`

// files.

// Directory for files generated by compiling test code.

This reverts commit 03eff91.

cpcallen · 2023-02-10T17:04:42Z

.eslintrc.js

@@ -0,0 +1,215 @@
+const rules = {
+  'curly': ['error'],


In TypeScript, it's best practice to quote keys you don't know at compile time.

TIL. Though I'm not sure that argument is exactly applicable here, since I'd guess that the format of the tsconfig data structure is probably fully defined somewhere in the tsc source code.

btw17 requested a review from a team as a code owner January 13, 2023 23:30

btw17 requested a review from cpcallen January 13, 2023 23:30

github-actions bot added the PR: feature Adds a feature label Jan 13, 2023

github-actions bot assigned cpcallen Jan 13, 2023

btw17 force-pushed the feat/add-ts-tests branch from cdd9888 to 464f700 Compare January 13, 2023 23:31

github-actions bot added PR: feature Adds a feature and removed PR: feature Adds a feature labels Jan 13, 2023

cpcallen requested changes Jan 18, 2023

View reviewed changes

cpcallen approved these changes Jan 20, 2023

View reviewed changes

btw17 force-pushed the feat/add-ts-tests branch 4 times, most recently from e4085f2 to 879bfd5 Compare January 27, 2023 16:49

cpcallen requested changes Feb 1, 2023

View reviewed changes

btw17 force-pushed the feat/add-ts-tests branch from 879bfd5 to 19f0a55 Compare February 3, 2023 19:00

cpcallen requested changes Feb 3, 2023

View reviewed changes

btw17 added 13 commits February 10, 2023 15:50

feat: added tests/typescript to test supported TS examples

417c651

fix: update the test name, description, and output

1990710

chore: remove unused imports in test_tasks.js

f9ebd19

fix: wrap README line at 80 characters

62043a8

fix: implemented different_user_input.ts feedback

42e3d60

fix: correct mistaken comments

995667f

chore: rename ./eslintrc.json to ./eslintrc.js

9908464

feat: added linting for tests/typescript

5aaca60

chore: cleanup eslintrc lines over 80 characters

688607e

fix: updated .eslintrc.js to provide an override for linting itself

c8d9faf

fix: updated tests to build to the build directory

29c9554

feat: updated gulp format to handle formatting .eslintrc.js

a497c8f

fix: updated .eslintrc.js to align with both formatter and linter

79d301a

btw17 added 3 commits February 10, 2023 15:50

fix: updated config comment wording

0da4ab3

fix: removed quotes for valid identifiers

ae1cd9f

Revert "fix: removed quotes for valid identifiers"

92d4dd8

This reverts commit 03eff91.

btw17 force-pushed the feat/add-ts-tests branch from 0e93012 to 92d4dd8 Compare February 10, 2023 15:51

cpcallen approved these changes Feb 10, 2023

View reviewed changes

cpcallen merged commit 13fe6ee into google:develop Feb 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: added `tests/typescript` to test supported TS examples #6775

feat: added `tests/typescript` to test supported TS examples #6775

btw17 commented Jan 13, 2023 •

edited

Loading

cpcallen left a comment

cpcallen Jan 17, 2023

btw17 Jan 18, 2023

btw17 Jan 20, 2023

btw17 commented Jan 20, 2023

cpcallen left a comment

cpcallen Feb 1, 2023

cpcallen Feb 1, 2023

btw17 Feb 3, 2023

cpcallen Feb 3, 2023

cpcallen Feb 3, 2023

btw17 Feb 3, 2023

btw17 Feb 3, 2023

cpcallen Feb 10, 2023

cpcallen Feb 3, 2023

btw17 Feb 3, 2023

cpcallen Feb 10, 2023

		// Directory where typescript compiler can be found for `./tests/typescript`
		// files.

	// Directory where typescript compiler can be found for `./tests/typescript`
	// files.
	// Directory for files generated by compiling test code.

feat: added tests/typescript to test supported TS examples #6775

feat: added tests/typescript to test supported TS examples #6775

Conversation

btw17 commented Jan 13, 2023 • edited Loading

The basics

The details

Resolves

Proposed Changes

Behavior Before Change

Behavior After Change

Reason for Changes

Test Coverage

Documentation

Additional Information

cpcallen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

btw17 commented Jan 20, 2023

cpcallen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: added `tests/typescript` to test supported TS examples #6775

feat: added `tests/typescript` to test supported TS examples #6775

btw17 commented Jan 13, 2023 •

edited

Loading