External runner fixes #24115
External runner fixes #24115
Conversation
…odules dirs so they dont participate in tests, sort errors
| @@ -27,9 +28,32 @@ abstract class ExternalCompileRunnerBase extends RunnerBase { | |||
| const testList = this.tests && this.tests.length ? this.tests : this.enumerateTestFiles(); | |||
|
|
|||
| describe(`${this.kind()} code samples`, () => { | |||
| const cwd = path.join(Harness.IO.getWorkspaceRoot(), this.testDir); | |||
| const placeholderName = ".node_modules"; | |||
There was a problem hiding this comment.
why not just add a types: [..] list?
There was a problem hiding this comment.
They have one already. That only says which things get auto-included without even doing an import, but the issue here is when i say const x = require("chalk") and we walk up the tree and find ours when we can't find it locally. 🌞
| try { | ||
| fs.renameSync(path.join(dir, moduleDirName), path.join(dir, placeholderName)); | ||
| } | ||
| catch { |
There was a problem hiding this comment.
why catch here, at least with nothing in the body?
There was a problem hiding this comment.
Don't care if the rename fails (since it means the folder didn't exist or we didn't have permissions to rename - in either case we're just going to continue)
There was a problem hiding this comment.
OK, as long as the failure is obvious (and linkable back to this line of code) some other way. I think in this case it will be obvious in the baselines.
| @@ -2,6 +2,7 @@ | |||
| /// <reference path="runnerbase.ts" /> | |||
| const fs = require("fs"); | |||
| const path = require("path"); | |||
| const del = require("del"); | |||
There was a problem hiding this comment.
So it's required before I renamed node_modules and make it unreachable.
| @@ -0,0 +1,12 @@ | |||
| Exit Code: 1 | |||
| Standard output: | |||
| node_modules/log4js/types/log4js.d.ts(4,2): error TS7008: Member 'getLogger' implicitly has an 'any' type. | |||
There was a problem hiding this comment.
You mean why didn't they?
There was a problem hiding this comment.
| @@ -91,7 +119,7 @@ class UserCodeRunner extends ExternalCompileRunnerBase { | |||
| // tslint:disable-next-line:no-null-keyword | |||
| return result.status === 0 && !result.stdout.length && !result.stderr.length ? null : `Exit Code: ${result.status} | |||
| Standard output: | |||
| ${stripAbsoluteImportPaths(result.stdout.toString().replace(/\r\n/g, "\n"))} | |||
| ${sortErrors(stripAbsoluteImportPaths(result.stdout.toString().replace(/\r\n/g, "\n")))} | |||
There was a problem hiding this comment.
does this change address the path changes (eg from E:/... to /opt/...)?
There was a problem hiding this comment.
They were already fixed by the stripAbsoluteImportPaths function - the only one that remained was one from outside the test directly, which shouldn't have been in the test in the first place (and is now gone).
* Add missing @types/node dep to so many projects, rename parent node_modules dirs so they dont participate in tests, sort errors * Accept new baselines * Satisfy linter
Follow up to #24100, which I'll now close.
Fixes #24104
Fixes #24105
The tests didn't need
typeRoots, unfortunately (that only affects implicit inclusion) - they needed all parentnode_modulesfully removed from the tree. (Which then showed how we were failing to include@types/nodeactually in a bunch of projects, which now do.) Also, the diagnostics are now sorted in case-sensitive path order (if they have a path - otherwise they should stay wherever they are - usually at the top), so the baseline file should match between platforms. I hope.