Fix broken user and docker tests #42431

rbuckton · 2021-01-21T02:52:19Z

Fix failing user tests due to peer dependency issues.

rbuckton · 2021-01-21T02:52:37Z

typescript-bot · 2021-01-21T02:52:41Z

Heya @rbuckton, I've started to run the parallelized community code test suite on this PR at 9978171. You can monitor the build here.

typescript-bot · 2021-01-21T03:33:42Z

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

sandersn

What about --legacy-peer-deps? That seems to be the intended workaround: https://blog.npmjs.org/post/626173315965468672/npm-v7-series-beta-release-and-semver-major

If this works, though, it's fine to merge it.

rbuckton · 2021-01-21T19:40:36Z

I debated between that and --force. I think either will work for now, but you are probably correct (as long as --legacy-peer-deps isn't removed at some point in the future). I was mostly concerned about npm version differences (i.e., if the tests were for some reason run on an earlier version of npm that doesn't yet support --legacy-peer-deps).

rbuckton · 2021-01-21T20:09:35Z

Ok, I think that's all of them. Need to run tests and accept new baselines.

@typescript-bot user test this

typescript-bot · 2021-01-21T20:09:40Z

Heya @rbuckton, I've started to run the parallelized community code test suite on this PR at 48232f6. You can monitor the build here.

rbuckton · 2021-01-21T20:10:36Z

@sandersn BTW, I switched to using --legacy-peer-deps but made it conditional on whether npm's major version is >= 7.

rbuckton · 2021-01-21T21:44:59Z

A few things:
There seems to be an error a the end of runTests:

Also, office-ui-fabric has (expected) failures since they haven't updated their version of tslib to the latest version. @weswigham, is that something we should do manually in the Dockerfile (since we also are overriding the version of TypeScript), or should we just leave it until they update?

weswigham · 2021-01-21T23:05:02Z

src/compiler/checker.ts

@@ -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);


Should we have a compiler test for this crash fix?

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.

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?

weswigham · 2021-01-21T23:06:40Z

src/testRunner/externalCompileRunner.ts

                        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;


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

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.

weswigham · 2021-01-21T23:07:22Z

tests/cases/docker/xterm.js/Dockerfile

@@ -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 :(


Should probably update this comment, too.

weswigham · 2021-01-21T23:08:10Z

tests/cases/docker/vscode/Dockerfile

@@ -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


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

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.

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

Nope, not according to the docs for yarn add.

weswigham · 2021-01-21T23:11:06Z

tests/cases/docker/prettier/Dockerfile

+RUN yarn
+ENTRYPOINT [ "yarn" ]
+# Build
+CMD [ "build" ]


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?

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

weswigham · 2021-01-21T23:12:59Z

is that something we should do manually in the Dockerfile (since we also are overriding the version of TypeScript), or should we just leave it until they update?

If it significantly blocks the baseline's usefulness, yeah, we can; otherwise I'd just accept the baseline with the errors until they patch the version themselves.

rbuckton · 2021-01-21T23:18:33Z

If it significantly blocks the baseline's usefulness, yeah, we can; otherwise I'd just accept the baseline with the errors until they patch the version themselves.

I'm unclear as to how I can accept user/docker test baselines. Can I do that from typescript-bot#137? If so, I don't have permission to merge changes in that repo. Or do I need to run them all locally and accept?

weswigham · 2021-01-22T00:23:35Z

You can pick the change the bot generates into a PR of your choosing or generate the baselines locally, yeah.

rbuckton · 2021-01-22T17:45:18Z

@weswigham any other feedback? If not I'd like to merge this so we can get the user tests working again.

weswigham

An isolated test for that crash big would still be nice.

rbuckton · 2021-01-22T21:25:44Z

I'll look into a repro, though I haven't had much luck so far tracking down what specific effects triggered it for the office-ui-fabric project.

Add --force to npm install script for user tests

9978171

typescript-bot assigned rbuckton Jan 21, 2021

typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jan 21, 2021

typescript-bot mentioned this pull request Jan 21, 2021

🤖 User test baselines have changed for fixUserTests typescript-bot/TypeScript#137

Open

sandersn reviewed Jan 21, 2021

View reviewed changes

rbuckton added 2 commits January 21, 2021 10:57

Migrate prettier to docker

c63c118

Fix vscode Dockerfile

28f25ab

rbuckton mentioned this pull request Jan 21, 2021

Maximum call stack size exceeded in isJSLiteralType #42445

Closed

rbuckton added 2 commits January 21, 2021 11:41

Fix stack space issue in isJSLiteralType

da62a37

Use --legacy-peer-deps based on npm version

a83a17e

rbuckton changed the title ~~Add --force to npm install script for user tests~~ Fix broken user and docker tests Jan 21, 2021

rbuckton marked this pull request as ready for review January 21, 2021 21:32

rbuckton requested review from weswigham and sandersn January 21, 2021 21:39

weswigham reviewed Jan 21, 2021

View reviewed changes

Fix xterm.js Dockerfile

8824950

rbuckton force-pushed the fixUserTests branch from 48232f6 to 8824950 Compare January 21, 2021 23:13

rbuckton mentioned this pull request Jan 22, 2021

Update user baselines #42451

Merged

weswigham approved these changes Jan 22, 2021

View reviewed changes

rbuckton merged commit ee3fe47 into master Jan 22, 2021

jakebailey deleted the fixUserTests branch November 7, 2022 17:38

		@@ -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 :(

Fix broken user and docker tests #42431

Fix broken user and docker tests #42431

Uh oh!

Conversation

rbuckton commented Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rbuckton commented Jan 21, 2021

Uh oh!

typescript-bot commented Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Jan 21, 2021

Uh oh!

sandersn left a comment

Choose a reason for hiding this comment

Uh oh!

rbuckton commented Jan 21, 2021

Uh oh!

rbuckton commented Jan 21, 2021

Uh oh!

typescript-bot commented Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rbuckton commented Jan 21, 2021

Uh oh!

rbuckton commented Jan 21, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weswigham Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weswigham commented Jan 21, 2021

Uh oh!

rbuckton commented Jan 21, 2021

Uh oh!

weswigham commented Jan 22, 2021

Uh oh!

rbuckton commented Jan 22, 2021

Uh oh!

weswigham left a comment

Choose a reason for hiding this comment

Uh oh!

rbuckton commented Jan 22, 2021

Uh oh!

Uh oh!

rbuckton commented Jan 21, 2021 •

edited

Loading

typescript-bot commented Jan 21, 2021 •

edited

Loading

typescript-bot commented Jan 21, 2021 •

edited

Loading

weswigham Jan 21, 2021 •

edited

Loading