-
Notifications
You must be signed in to change notification settings - Fork 149
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
[ACS-6510] playwright List View e2e test #3566
[ACS-6510] playwright List View e2e test #3566
Conversation
…stview-personal-files
} | ||
|
||
test.describe('Remember sorting', () => { | ||
interface NodesIds { |
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.
Why do we need this interface? It can be a simple array of strings instead I think
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.
removed this interface
} from '@alfresco/playwright-shared'; | ||
|
||
test.describe('Remember sorting', () => { | ||
interface NodesIds { |
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.
Same here
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.
removed this interface.
timeouts | ||
} from '@alfresco/playwright-shared'; | ||
|
||
test.describe('Remember sorting', () => { |
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.
Is it possible to run those tests together with ones related to login in the upper file? The setup is quite similar and in this way we can reuse some setup code
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.
It was getting some failure due to logout related test, now tries some approach and add code in one file.
parentId: string = '-my-', | ||
title: string = '', | ||
description: string = '' | ||
): Promise<NodeEntry | null> { |
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.
If you want to return Promise I wouldn't just return null, you can return rejected promise instead and catch it whenever you use the method
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.
removed and added reject promise
majorVersion: boolean = true, | ||
comment?: string, | ||
newName?: string | ||
): Promise<NodeEntry | null> { |
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.
Same here for null
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.
removed and added reject promise
if (isEmpty) { | ||
return this.emptyListTitle.innerText(); | ||
} | ||
return ''; |
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.
It can be shorter like: return await this.isEmpty() ? this.emptyListTitle.innerText() : '';
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.
added suggested changes. looks good
if (isEmpty) { | ||
return this.emptyListSubtitle.innerText(); | ||
} | ||
return ''; |
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.
Can be shorter as well like in the example above
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.
added suggested changes. looks good
if (isEmpty) { | ||
return this.getChild('adf-custom-empty-content-template').innerText(); | ||
} | ||
return ''; |
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.
Same here
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.
added suggested changes. looks good
@@ -166,4 +166,12 @@ export class PaginationComponent extends BaseComponent { | |||
async closeMenu(): Promise<void> { | |||
await this.page.keyboard.press('Escape'); | |||
} | |||
|
|||
async isRangePresent() { |
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.
Can you specify the return type?
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.
added return promise type
return this.range.isVisible(); | ||
} | ||
|
||
async isMaxItemsPresent() { |
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.
Can you specify the return type?
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.
added return promise type
Quality Gate passedKudos, no new issues were introduced! 0 New issues |
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (check one with "x")
What is the current behaviour? (You can also link to an open issue here)
protractor List View e2e Test
What is the new behaviour? new playwright test
Does this PR introduce a breaking change? (check one with "x")
If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...
Other information: https://alfresco.atlassian.net/browse/ACS-6510