chore: lint for floating promises #634
Conversation
microbit-matt-hillsdon
requested review from
maribethb
and removed request for
a team
tsconfig.json
Outdated
| // Only src matters for production builds. | ||
| "include": ["./src", "./test"], | ||
| "exclude": ["./test/webdriverio/"] | ||
| "exclude": ["./test/webdriverio/test"] |
There was a problem hiding this comment.
Before this change, test/webdriverio/index.ts was covered by neither tsconfig.json file.
There was a problem hiding this comment.
I'm removing this line in #631 as we don't want the test output in the published package. Does that file need to be covered by one of the tsconfig.json files? I think since webpack is compiling it it doesn't, but I haven't looked in depth at how the wdio tests are set up -- I'm surprised to see we invoke tsc directly instead of letting webpack handle it.
There was a problem hiding this comment.
I think the tsc build is for the wdio tests themselves rather than the app under test.
Your change leaves the demo app and the test app used for wdio tests without a matching tsconfig. I think they both should have one.
The downside is that tooling might use default configuration (e.g. IDEs, not sure what your webpack config does).
It came up on this PR because the eslint rules that need type information (like the floating promise ones) choose to fail hard in this scenario:
/Users/mth/Development/microbit/makecode/blockly-keyboard-experimentation/test/index.ts
0:0 error Parsing error: ESLint was configured to run on `<tsconfigRootDir>/test/index.ts` using `parserOptions.project`: /users/mth/development/microbit/makecode/blockly-keyboard-experimentation/tsconfig.json
However, that TSConfig does not include this file. Either:
- Change ESLint's list of included files to not include this file
- Change that TSConfig to include this file
- Create a new TSConfig that includes this file and include it in your parserOptions.project
See the typescript-eslint docs for more info: https://typescript-eslint.io/linting/troubleshooting#i-get-errors-telling-me-eslint-was-configured-to-run--however-that-tsconfig-does-not--none-of-those-tsconfigs-include-this-file
There are equivalent errors for the other app files.
I think it is made more confusing than it needs to be by nesting all the projects inside each other. There is a demo/test app in "test/" containing another app used for the webdriver io tests in webdriverio/ which in turn contains the actual tests in another "test/".
I think if this was flattened into e.g. "demo", "wdio/app" and "wdio/tests" where "wdio" itself was empty and each of those folders contained a tsconfig it would be less puzzling and each could have a tsconfig with no need for excludes.
I'll revisit this later in the week, it likely isn't worth the churn now.
There was a problem hiding this comment.
OK, thanks for the info. In blockly-samples the test page (which I think is what you're calling the demo app) are not covered by our tsconfig. Maybe that's an error with our setup for the other plugins, but it works because the webpack config compiles the test directory anyway (for development and test mode) and we never invoke tsc directly.
Flattening out the structure of the test directory seems fine, but will think harder about it later when the release is done.
There was a problem hiding this comment.
I've pushed and rebased a simplified version of this change that only adds the new lint checks to the wdio tests to decouple it from the tsconfig.json discussion because on reflection I don't think it's that helpful for me to propose changes here.
I'd tried to do sometime simpler like this initially and somehow convinced myself that a bigger config change was needed but the different sections with different file globs combine just as you'd hope.
|
Fixed conflicts. |
microbit-matt-hillsdon
force-pushed
the
floating-promises
branch
from
6bf1e6d to
4944785
Compare
Fix missing awaits in the tests. Needed tsconfig information so I've added the project setting - see https://typescript-eslint.io/blog/parser-options-project-true/#introducing-true I've done this just for the tests as otherwise we hit issues with the test app and the webdriver test app not being covered by tsconfig.json files. It could be trivially extended to the src tree.
microbit-matt-hillsdon
force-pushed
the
floating-promises
branch
from
4944785 to
440a6bc
Compare
|
@microbit-matt-hillsdon Is this ready to be reviewed again? I had been thinking that you said you would hold off on it, but re-reading the latest comments I might have been mistaken. Sorry for letting it sit! |
Yes, I think this is good to review now and narrower in scope than when previously reviewed. |
maribethb
left a comment
maribethb
left a comment
There was a problem hiding this comment.
Thanks, this is much more manageable from a config perspective and will definitely make tests less flaky!
2 participants
Fix missing awaits in the webdriver tests.
Needed tsconfig information so I've added the project setting - see https://typescript-eslint.io/blog/parser-options-project-true/#introducing-true
It's all but inevitable that folks miss some awaits in webdriver tests and it's easy to mask the issue most of the time with a pause leading to tests that are both slow and flaky.
This is just a suggestion, feel free to close it if the team finds this to be more trouble than its worth and I'll open a version that just fixes the missing awaits we have right now. I personally find this kind of support really useful when writing async code like the webdriver tests.
Rules: