-
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
[identity] Re-enable tests #29778
[identity] Re-enable tests #29778
Conversation
@@ -23,7 +23,7 @@ describe("getBearerTokenProvider", function () { | |||
const scope = "https://vault.azure.net/.default"; | |||
|
|||
it("returns a callback that returns string tokens", async function () { | |||
const credential = new EnvironmentCredential(recorder.configureClientOptions({})); | |||
const credential = new DefaultAzureCredential(recorder.configureClientOptions({})); |
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 think through this a bit. But it's probably fine for now...
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.
Yeah how do we know which cred it picks up in the unit tests?
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.
Just wanted to say that I agree with your question 😄 it's not clear or deterministic.
Now that CI / recorded tests are running and green again, I plan to review our tests in live mode. I'll revisit this one and choose a more specific credential as part of that. Thanks for following up with me offline!
ssh-keygen -t rsa -b 4096 -f $PSScriptRoot/sshKey -N '' -C '' | ||
$sshKey = Get-Content $PSScriptRoot/sshKey.pub | ||
|
||
$templateFileParameters['sshPubKey'] = $sshKey |
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.
This is a required item in the template, so we should be generating it always. Otherwise, you cannot deploy test resources
@@ -121,12 +121,12 @@ export async function msalNodeTestSetup( | |||
{ | |||
regex: true, | |||
target: `x-client-OS=[a-zA-Z0-9]+`, | |||
value: `x-client-OS=x-client-OS`, | |||
value: `x-client-OS=Sanitized`, |
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.
This really ended up being the only diff.., It was getting double-sanitized so replacing it with something that is not the key itself prevents it. I don't understand why or how it works, but let's get tests running again 😄
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.
Maybe double sanitized, because we might be calling the same function twice at 2 different places - one might be getting called from inside of another file?
API change check API changes are not detected in this pull request. |
Re-enabled tests in playback mode, including better filtering in live mode to skip the MI tests