-
Notifications
You must be signed in to change notification settings - Fork 0
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
More command tests #23
Conversation
This adds testing for all commands except SSH.
a15e44c
to
5841f22
Compare
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.
LGTM, just a handful of questions to help improve my understanding of the changes.
try { | ||
await spec.fail((_, err) => (error = err)).parse(command); | ||
} catch (thrown: any) { | ||
error = thrown; |
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.
could this function only returning the thrown
error in the catch block instead of defining let error: any
at the top?
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.
No, because the main throw path is via line 6 (TBH one could probably just remove the try / catch altogether; I'll investigate)
src/commands/login.ts
Outdated
/** Logs in the user | ||
* | ||
* Currently only supports login to a single organization. Login credentials, together | ||
* with organization details, are saved to ~/.p0/identity.json. |
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.
nit: should we mention that the file path is determined by the variable: IDENTITY_FILE_PATH
instead of a hard coded path?
}); | ||
}); | ||
}); | ||
describe("with Okta SAML", () => { |
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 noticed that we're missing a test here to verify the error No roles found. You have roles on these accounts...
which is thrown when we're trying to list roles but the user doesn't have any.
p0cli/src/commands/aws/role.ts
Line 128 in 77e650d
throw `No roles found. You have roles on these accounts:\n${accounts.join( |
We can probably add it later, wdyt?
8c65215
to
8584764
Compare
8584764
to
e755965
Compare
This adds testing for all commands except SSH.