test(instrumentation-http): avoid deprecated url.parse()#6102
test(instrumentation-http): avoid deprecated url.parse()#6102pichlermarc merged 2 commits intoopen-telemetry:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6102 +/- ##
=======================================
Coverage 95.16% 95.16%
=======================================
Files 316 316
Lines 9220 9220
Branches 2080 2080
=======================================
Hits 8774 8774
Misses 446 446 🚀 New features to boost your workflow:
|
| hostname: null, | ||
| hash: null, | ||
| search: null, | ||
| query: null as unknown as undefined, |
There was a problem hiding this comment.
Unrelated note - the types for getAbsoluteUrl() seem like they may be slightly incorrect. See the other type cast on line 203 of this file.
url.parse() returns an object whose query field is of type string | null | ParsedUrlQuery. However, string and null are problematic in getAbsoluteUrl(). The first argument to getAbsoluteUrl() is currently ParsedRequestOptions | null. ParsedRequestOptions is defined as:
export type ParsedRequestOptions =
| (http.RequestOptions & Partial<url.UrlWithParsedQuery>)
| http.RequestOptions;And, the relevant Node core types are:
// Output of `url.parse`
interface Url {
auth: string | null;
hash: string | null;
host: string | null;
hostname: string | null;
href: string;
path: string | null;
pathname: string | null;
protocol: string | null;
search: string | null;
slashes: boolean | null;
port: string | null;
query: string | null | ParsedUrlQuery;
}
interface UrlWithParsedQuery extends Url {
query: ParsedUrlQuery;
}
interface UrlWithStringQuery extends Url {
query: string | null;
}It's not clear to me why ParsedRequestOptions uses url.UrlWithParsedQuery instead of url.Url here.
There was a problem hiding this comment.
I tired to do some archeology on this and it seems that this was added all the way back in #161 and has grown to be somewhat different since then. I think some cleanup is needed here (in a separate PR).
| hostname: null, | ||
| hash: null, | ||
| search: null, | ||
| query: null as unknown as undefined, |
There was a problem hiding this comment.
I tired to do some archeology on this and it seems that this was added all the way back in #161 and has grown to be somewhat different since then. I think some cleanup is needed here (in a separate PR).
Which problem is this PR solving?
The tests in the
instrumentation-httppackage rely onurl.parse(), which is deprecated in Node.Fixes #6061
Short description of the changes
The relevant tests have been updated to no longer rely on
url.parse(). Most of the uses ofurl.parse()are simply replaced bynew URL()andurl. urlToHttpOptions().Some of the tested APIs, such as
getRequestInfo()andgetAbsoluteUrl()seem to specifically expect objects that are output fromurl.parse(). In these cases, I inlined the result ofurl.parse()in the tests since this is highly unlikely to ever change in Node.js at this point.Type of change
How Has This Been Tested?
Checklist: