build: make tests run faster #4205
Conversation
|
Here is the summary of changes. You are about to add 3 region tags.
You are about to delete 3 region tags.
This comment is generated by snippet-bot.
|
sofisl
changed the title
make-change-to-package.cjs
Outdated
| const fs = require('fs'); | ||
| const path = require('path'); | ||
| const cp = require('child_process'); | ||
| const dirsToSkip = ['gapic-node-templating', 'typeless-sample-bot']; |
There was a problem hiding this comment.
what purpose does this script serve?
There was a problem hiding this comment.
I will delete it! Shouldn't be in here. I'll wait once this is approved to delete it so tests won't rerun.
| @@ -57,6 +58,7 @@ | |||
| "jsdoc-fresh": "^2.0.0", | |||
| "jsdoc-region-tag": "^2.0.0", | |||
| "linkinator": "^4.0.0", | |||
| "long": "^5.2.3", | |||
There was a problem hiding this comment.
I don't think we should need to add this long dependency, as it's pulled in by google-gax. I'm curious why this was needed.
There was a problem hiding this comment.
The way pnpm installs dependencies doesn't grab this dependency. It's really only for this line. I spoke with @alexander-fenster about this, and he recommended adding it as a devDependency, over skipping type checking in the tsconfig.
I think this is actually a benefit, it shows that pnpm is not implicitly grabbing dependencies in the way npm was, since we are actually using long since it is used in protos.
There was a problem hiding this comment.
commenting these tests out per b/278007126
There was a problem hiding this comment.
This is looking really good to me.
The main thing that jumps out at me, is I'd rather avoid the make-change-to-package.cjs which feels a little magical (where is this actually invoked?).
I like that you did not have to actually switch the package.json itself to using pnpm. Did you notice a significant performance improvement using pnpm rather than npm?
Partially addresses #4140
preparescript will do that as wellpnpminstead ofnpm