feat(chat-e2e): toolset oauth regression tests #5232
Conversation
|
/deploy-review
|
|
|
||
| export class ToolsetApiHelper extends BaseApiHelper { | ||
| public async createToolset(toolsetModel: Toolset) { | ||
| const url = `${API.toolsetCreateHost()}/${this.userBucket ?? BucketUtil.getBucket()}/${toolsetModel.display_name}${ItemUtil.entityIdSeparator}${toolsetModel.display_version}`; |
There was a problem hiding this comment.
Consider breaking it into multiple lines, it is hard to read. I also do the same way, but probably we need to stop.
Recommendation: Consider breaking it into multiple lines
const bucket = this.userBucket ?? BucketUtil.getBucket();
const entityPath = `${toolsetModel.display_name}${ItemUtil.entityIdSeparator}${toolsetModel.display_version}`;
const url = `${API.toolsetCreateHost()}/${bucket}/${entityPath}`;
`
| export class OAuthMockHelper { | ||
| private page: Page; | ||
| private readonly initialToolset: Toolset; | ||
| private initialToolset: Toolset; |
There was a problem hiding this comment.
The name initialToolset suggests immutability, but it's now mutable
Rename to currentToolset or toolset
| } | ||
|
|
||
| public async setupToolsetRoutes(): Promise<void> { | ||
| public async setupUpdatedToolsetRoutes( |
There was a problem hiding this comment.
What happens to previously merged changes if this is called multiple times?
We need a jsdoc here, smth like: (AI generated)
/**
* Updates the toolset routes with partial toolset properties.
* Merges the provided properties with the current toolset state.
* Note: Subsequent calls will merge with the already-modified toolset.
* @param updatedToolsetProps - Partial toolset properties to merge
*/
|
|
||
| public async setupSignOutRoute(): Promise<void> { | ||
| const signOutUrl = `**${API.api}/ops/${ServerSlugs.TOOLSET_SIGN_OUT}`; | ||
| // Intercept OAuth redirect |
There was a problem hiding this comment.
But it's intercepting sign-out, isn't it?
|
|
||
| export enum OAuthOptions { | ||
| WithLogin = 'With login', | ||
| WithConfig = 'With login & config', |
There was a problem hiding this comment.
Please rename WithConfig to the WithLoginAndConfig
Add try-catch or validation?
| // Intercept OAuth redirect | ||
| await this.page.route(signOutUrl, async (route, request) => { | ||
| this.state.isSignedIn = false; | ||
| this.state.signOutRequest = request.postDataJSON(); |
There was a problem hiding this comment.
Could throw unhandled exception
| await this.waitForExpectedResponses( | ||
| () => this.navigateToUrl(entityEditorAttributes.entityEditorPath), | ||
| expectedResponses, | ||
| private async openEditEntityPage( |
There was a problem hiding this comment.
openEditToolsetPage() is public, but openEditEntityPage() is private
why? Looks like it was an intention, but I have to ask
| AddToolsetSettingsFormSelector.authDetailsContainer, | ||
| ); | ||
| public oAuthOptions = this.authDetailsContainer | ||
| .getChildElementBySelector(Tags.label) |
There was a problem hiding this comment.
Could you add a data-qa locator to the React element?
| await oauthMockHelper.cleanup(); | ||
| }); | ||
|
|
||
| async function verifySuccessfulLogin() { |
There was a problem hiding this comment.
Extract to shared helper or assertion method
| .filter({ has: this.page.getByRole('radio') }); | ||
| public oAuthOption = (loginOption: OAuthOptions) => | ||
| this.oAuthOptions.locator(`[${Attributes.id}="${loginOption}"]`); | ||
| public signInButton = this.authDetailsContainer.getChildElementBySelector( |
There was a problem hiding this comment.
BTW, probably we need to rename all the sign in selectors to "log in"? Because the button title is "Log in"
Description:
implemented regression tests for toolset oauth
Checklist:
fix(<scope>):,feat(<scope>):,feature(<scope>):,chore(<scope>):,hotfix(<scope>):ore2e(<scope>):. If contains breaking changes then the pull request name must start withfix(<scope>)!:,feat(<scope>)!:,feature(<scope>)!:,chore(<scope>)!:,hotfix(<scope>)!:ore2e(<scope>)!:where<scope>is name of affected project:chat,chat-e2e,overlay,shared,sandbox-overlay, etc.(Issue #<TICKET_ID>)(comma-separated list of issues)