-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[EngSys] upgrade dev dependency puppeteer
to ^22.0.0
#28200
Changes from 14 commits
d7971cc
b74940c
75342da
9169ae4
a5fe322
094a17f
a5d4515
f1730b5
3ab2efe
71ff357
d6ecc7a
1ae7e34
ac0d889
eeec0db
a735c93
9705ed4
5e590de
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,7 +31,7 @@ import { addTransform, Transform } from "./transform"; | |
import { createRecordingRequest } from "./utils/createRecordingRequest"; | ||
import { logger } from "./log"; | ||
import { setRecordingOptions } from "./options"; | ||
import { isNode } from "@azure/core-util"; | ||
import { isBrowser, isNode } from "@azure/core-util"; | ||
import { env } from "./utils/env"; | ||
import { decodeBase64 } from "./utils/encoding"; | ||
import { AdditionalPolicyConfig } from "@azure/core-client"; | ||
|
@@ -52,6 +52,7 @@ export class Recorder { | |
private sessionFile?: string; | ||
private assetsJson?: string; | ||
private variables: Record<string, string>; | ||
private matcherSet = false; | ||
|
||
constructor(private testContext?: Test | undefined) { | ||
logger.info(`[Recorder#constructor] Creating a recorder instance in ${getTestMode()} mode`); | ||
|
@@ -216,6 +217,12 @@ export class Recorder { | |
* - sanitizerOptions - Generated recordings are updated by the "proxy-tool" based on the sanitizer options provided, these santizers are applied only in "record" mode. | ||
*/ | ||
async start(options: RecorderStartOptions): Promise<void> { | ||
if (isBrowser && isPlaybackMode()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Move this to a new method "preStart()" or similar, and call that here to keep the code clean? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated |
||
if (!this.matcherSet) { | ||
await this.setMatcher("CustomDefaultMatcher"); | ||
this.matcherSet = true; | ||
} | ||
} | ||
if (isLiveMode()) return; | ||
logger.info(`[Recorder#start] Starting the recorder in ${getTestMode()} mode`); | ||
this.stateManager.state = "started"; | ||
|
@@ -379,13 +386,33 @@ export class Recorder { | |
/** | ||
* Sets the matcher for the current recording to the matcher specified. | ||
*/ | ||
async setMatcher(matcher: Matcher, options?: CustomMatcherOptions): Promise<void> { | ||
async setMatcher(matcher: Matcher, options: CustomMatcherOptions = {}): Promise<void> { | ||
if (isPlaybackMode()) { | ||
if (!this.httpClient) { | ||
throw new RecorderError("httpClient should be defined in playback mode"); | ||
} | ||
|
||
await setMatcher(Recorder.url, this.httpClient, matcher, this.recordingId, options); | ||
const excludedHeaders = isBrowser | ||
? (options.excludedHeaders ?? []).concat("Accept-Language") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a comment explaining why? for future reference There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
: options.excludedHeaders; | ||
|
||
const updatedOptions = { | ||
...options, | ||
excludedHeaders, | ||
}; | ||
if (matcher === "BodilessMatcher") { | ||
updatedOptions.compareBodies = false; | ||
await setMatcher( | ||
Recorder.url, | ||
this.httpClient, | ||
"CustomDefaultMatcher", | ||
this.recordingId, | ||
updatedOptions, | ||
); | ||
} else { | ||
await setMatcher(Recorder.url, this.httpClient, matcher, this.recordingId, updatedOptions); | ||
} | ||
this.matcherSet = true; | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were changes in the recorder related to the upgrade or something found in passing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see #28455 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes there's some behavior changes in headers being sent (Accept-Language). I updated sanitizers in recorder and keyvault. However some keyvault tests still failed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to re-record keyvault tests with the new service version some time in the next few days (it's not fully deployed yet). Hopefully just rerecording will fix the problem but if not I will investigate further at that time :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I am curious what the change to
Accept-Language
was. I remember this being a bugbear for me when playing back tests in the past since the locale on my machine wasen-CA
instead ofen-US
which is what was present in the recordings.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC ideally the header should be ignored by test-proxy. Not sure what was the exact issue that prevented it from happening.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We wanted to do this here Azure/azure-sdk-tools#6152 but didn't in the end, presumably due to engsys objection. @HarshaNalluru do you recall why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Their resistance was that it might have unintended consequences and is disruptive keeping other languages as well in mind.
From JS perspective, it should be safe, and we need to move forward with this change, so doing it in JS recorder makes sense.