Skip to content

Conversation

@Hweinstock
Copy link
Contributor

@Hweinstock Hweinstock commented Feb 17, 2025

Problem

#6594

The getMfaCodeFromUser currently uses a deprecated version of createInputBox and the following promptUser function:

export async function promptUser({
inputBox,
onValidateInput,
onDidTriggerButton,
}: {
inputBox: vscode.InputBox
onValidateInput?(value: string): string | undefined
onDidTriggerButton?(
button: vscode.QuickInputButton,
resolve: (value: string | PromiseLike<string | undefined> | undefined) => void,
reject: (reason?: any) => void
): void
}): Promise<string | undefined> {
const disposables: vscode.Disposable[] = []
try {
const response = await new Promise<string | undefined>((resolve, reject) => {
inputBox.onDidAccept(
() => {
if (!inputBox.validationMessage) {
resolve(inputBox.value)
}
},
inputBox,
disposables
)
inputBox.onDidHide(
() => {
resolve(undefined)
},
inputBox,
disposables
)
if (onValidateInput) {
inputBox.onDidChangeValue(
(value) => {
inputBox.validationMessage = onValidateInput(value)
},
inputBox,
disposables
)
}
if (onDidTriggerButton) {
inputBox.onDidTriggerButton(
(btn: vscode.QuickInputButton) => onDidTriggerButton(btn, resolve, reject),
inputBox,
disposables
)
}
inputBox.show()
})
return response
} finally {
for (const d of disposables) {
d.dispose() as void
}
inputBox.hide()
}
}

Based on the stack trace, there is a race condition here that can lead to a type error, but unsure how?
Stack trace:

TypeError: inputBox.hide is not a function
      at promptUser (/Users/runner/work/aws-toolkit-vscode/aws-toolkit-vscode/packages/core/src/shared/ui/input.ts:133:18)
      at async getMfaTokenFromUser (/Users/runner/work/aws-toolkit-vscode/aws-toolkit-vscode/packages/core/src/auth/credentials/utils.ts:124:19)
      at async /Users/runner/work/aws-toolkit-vscode/aws-toolkit-vscode/packages/core/src/auth/providers/sharedCredentialsProvider.ts:422:38

Investigation Notes:

  • The error only happens when running the test suite as a whole.
  • The error only happens in CI (unable to reproduce locally).

Solution

  • avoid use of deprecated createInputBox function, prefer new version instead.
  • Ran the tests 4x with 1000x on the test, and did not see a failure.

Future Work

  • Migrate existing cases away from this deprecated createInputBox if there is evidence it leads to flaky tests or unreliable behavior.

  • Treat all work as PUBLIC. Private feature/x branches will not be squash-merged at release time.
  • Your code changes must meet the guidelines in CONTRIBUTING.md.
  • License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@Hweinstock Hweinstock changed the title test(auth): investigating flaky MFA test (IGNORE) test(auth): investigating flaky MFA test (WIP) Feb 17, 2025
@Hweinstock Hweinstock changed the title test(auth): investigating flaky MFA test (WIP) test(auth): avoid deprecated ui constructor that caused flakiness. Feb 17, 2025
@Hweinstock Hweinstock changed the title test(auth): avoid deprecated ui constructor that caused flakiness. test(auth): avoid deprecated InputBox usage that caused flakiness. Feb 17, 2025
@Hweinstock Hweinstock marked this pull request as ready for review February 17, 2025 20:17
@Hweinstock Hweinstock requested a review from a team as a code owner February 17, 2025 20:17
@github-actions

This comment was marked as off-topic.

@jpinkney-aws
Copy link
Contributor

@Hweinstock
Copy link
Contributor Author

Yea, I was digging through that code a bit, and suspect its making some assumptions about how we create our prompters. Its still unclear to me how the old createInputBox break those assumptions.

@justinmk3 justinmk3 merged commit c892f7f into aws:master Feb 18, 2025
26 checks passed
@Hweinstock Hweinstock deleted the flaky/authMFA branch February 18, 2025 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants