Skip to content

Fix broken user and docker tests #42431

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

Merged
merged 6 commits into from
Jan 22, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14099,7 +14099,8 @@ namespace ts {
return some((type as IntersectionType).types, isJSLiteralType);
}
if (type.flags & TypeFlags.Instantiable) {
return isJSLiteralType(getResolvedBaseConstraint(type));
const constraint = getResolvedBaseConstraint(type);
return constraint !== type && isJSLiteralType(constraint);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have a compiler test for this crash fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm honestly not sure how to trigger it. It was triggered by something using typescript to parse just.config.ts in office-ui-fabric, and I'm not sure how deep the rabbit hole goes. At least its verified by the fact that office-ui-fabric is compiling again.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Off the top of my head, I know pattern template literals are returned as their own constraint by computeBaseConstraint, and are.... Kinda recent? So maybe there's a situation with a template triggering it?

}
return false;
}
Expand Down
10 changes: 7 additions & 3 deletions src/testRunner/externalCompileRunner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,31 +68,35 @@ namespace Harness {

cwd = config.path ? path.join(cwd, config.path) : submoduleDir;
}
const npmVersionText = exec("npm", ["--version"], { cwd, stdio: "pipe" })?.trim();
const npmVersion = npmVersionText ? ts.Version.tryParse(npmVersionText.trim()) : undefined;
const isV7OrLater = !!npmVersion && npmVersion.major >= 7;
if (fs.existsSync(path.join(cwd, "package.json"))) {
if (fs.existsSync(path.join(cwd, "package-lock.json"))) {
fs.unlinkSync(path.join(cwd, "package-lock.json"));
}
if (fs.existsSync(path.join(cwd, "node_modules"))) {
del.sync(path.join(cwd, "node_modules"), { force: true });
}
exec("npm", ["i", "--ignore-scripts"], { cwd, timeout: timeout / 2 }); // NPM shouldn't take the entire timeout - if it takes a long time, it should be terminated and we should log the failure
exec("npm", ["i", "--ignore-scripts", ...(isV7OrLater ? ["--legacy-peer-deps"] : [])], { cwd, timeout: timeout / 2 }); // NPM shouldn't take the entire timeout - if it takes a long time, it should be terminated and we should log the failure
}
const args = [path.join(IO.getWorkspaceRoot(), "built/local/tsc.js")];
if (types) {
args.push("--types", types.join(","));
// Also actually install those types (for, eg, the js projects which need node)
if (types.length) {
exec("npm", ["i", ...types.map(t => `@types/${t}`), "--no-save", "--ignore-scripts"], { cwd: originalCwd, timeout: timeout / 2 }); // NPM shouldn't take the entire timeout - if it takes a long time, it should be terminated and we should log the failure
exec("npm", ["i", ...types.map(t => `@types/${t}`), "--no-save", "--ignore-scripts", ...(isV7OrLater ? ["--legacy-peer-deps"] : [])], { cwd: originalCwd, timeout: timeout / 2 }); // NPM shouldn't take the entire timeout - if it takes a long time, it should be terminated and we should log the failure
}
}
args.push("--noEmit");
Baseline.runBaseline(`${cls.kind()}/${directoryName}.log`, cls.report(cp.spawnSync(`node`, args, { cwd, timeout, shell: true }), cwd));

function exec(command: string, args: string[], options: { cwd: string, timeout?: number }): void {
function exec(command: string, args: string[], options: { cwd: string, timeout?: number, stdio?: import("child_process").StdioOptions }): string | undefined {
const res = cp.spawnSync(isWorker ? `${command} 2>&1` : command, args, { shell: true, stdio, ...options });
if (res.status !== 0) {
throw new Error(`${command} ${args.join(" ")} for ${directoryName} failed: ${res.stdout && res.stdout.toString()}`);
}
return options.stdio === "pipe" ? res.stdout.toString("utf8") : undefined;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't actually use this returned value anywhere, do we?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm using it to test the npm version, since --legacy-peer-deps is new in npm 7, so if you're running tests on an older NodeJS/npm that flag would result in an error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh, ok

}
});
});
Expand Down
13 changes: 13 additions & 0 deletions tests/cases/docker/prettier/Dockerfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
FROM node:current
RUN npm i -g yarn --force
RUN git clone https://github.com/prettier/prettier.git /prettier
WORKDIR /prettier
RUN git pull
COPY --from=typescript/typescript /typescript/typescript-*.tgz /typescript.tgz
RUN mkdir /typescript
RUN tar -xzvf /typescript.tgz -C /typescript
RUN yarn add typescript@/typescript.tgz
RUN yarn
ENTRYPOINT [ "yarn" ]
# Build
CMD [ "build" ]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, so the user test previously just applied a custom tsconfig to the contents of the src directory and built it. That actually includes way more than they typecheck in their own build: https://github.com/prettier/prettier/blob/master/tsconfig.json Do we want to continue checking the whole src folder under our own rules like the user test used to, or should we use their build like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably use their build, since that's what they're running.

1 change: 1 addition & 0 deletions tests/cases/docker/vscode/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ COPY --from=typescript/typescript /typescript/typescript-*.tgz /typescript.tgz
WORKDIR /vscode/build
RUN yarn add typescript@/typescript.tgz
WORKDIR /vscode/extensions
RUN yarn add rimraf
Copy link
Member

@weswigham weswigham Jan 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this working around a bug in vscode's own build? Since we clearly don't use rimraf here in the dockerfile.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The vscode build is failing because they have a postinstall.js script that runs rimraf to clean up TypeScript files they don't need, but we're running yarn add for our copy of TypeScript before all the dependencies are installed which triggers the postinstall script.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does yarn have a --ignore-scripts equivalent we should use instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, not according to the docs for yarn add.

RUN yarn add typescript@/typescript.tgz
WORKDIR /vscode
RUN yarn add typescript@/typescript.tgz
Expand Down
4 changes: 2 additions & 2 deletions tests/cases/docker/xterm.js/Dockerfile
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# node-pty doesn't build on node 12 right now, so we lock to 8 - the version xterm itself tests against :(
FROM node:8
# node-pty doesn't build on node 12 right now, so we lock to 10
FROM node:10
RUN git clone https://github.com/xtermjs/xterm.js.git /xtermjs
WORKDIR /xtermjs
RUN git pull
Expand Down
4 changes: 0 additions & 4 deletions tests/cases/user/prettier/test.json

This file was deleted.

17 changes: 0 additions & 17 deletions tests/cases/user/prettier/tsconfig.json

This file was deleted.