fix(core): discovery url handling, error code review helper, and pack-n-play test timeouts - #9061
fix(core): discovery url handling, error code review helper, and pack-n-play test timeouts#9061bshaffer wants to merge 5 commits into
Conversation
…-n-play test timeouts
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates test timeouts and npm install flags in pack-n-play, replaces the deprecated url.parse with the modern URL API in nodejs-googleapis-common, and introduces a helper to extract error codes from nested errors in gcp-metadata. The review feedback highlights a potential issue in gcp-metadata where a wrapper error containing an AggregateError as its cause would fail to resolve properly and return 'UNKNOWN'. A recursive unwrapping function is suggested to handle this robustly.
| cwd: dir, | ||
| }; | ||
| await execa('npm', ['install'], opts); | ||
| await execa('npm', ['install', '--no-audit', '--no-fund'], opts); |
There was a problem hiding this comment.
I guess we should always do this for non-interactive npm installs, since we don't do anything with this info?
There was a problem hiding this comment.
Yes i believe so, it's a small optimization
| 'Content-Type': 'application/json', | ||
| }); | ||
| const disco = new Discovery({}); | ||
| const makeEndpoint = await disco.discoverAPI(discoUrl); |
There was a problem hiding this comment.
I think discoverAPI used to handle URLs without ports, so we should keep that part of the test (even if we need to pass the port to nock.)
Unless the test never passed?!
There was a problem hiding this comment.
the reason we added the port is we switched to using new URL() API from the deprecated url.parse(), and the new URL() API requires a port.
There was a problem hiding this comment.
I'm not sure new URL() requires a port: https://nodejs.org/api/url.html#new-urlinput-base
If it did, I think we would need to update discoverAPI to infer a port based on the scheme (or modify the callers to make sure they pass a port)
I do think nock might need one. So it might be more correct to pass a port to nock and skip the port for discoverAPI.
| }); | ||
| it('should discover an API through second weird path', async () => { | ||
| const discoUrl = 'http://test.local'; | ||
| const discoUrl = 'http://test.local:80'; |
There was a problem hiding this comment.
Same as above, the reason we added the port is we switched to using new URL() API from the deprecated url.parse(), and the new URL() API requires a port.
2 participants
Separated from #9058 to isolate Core library and pack-n-play CI fixes: