Skip to content
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

add create, delete book scenario #1

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Conversation

vl-leon
Copy link
Collaborator

@vl-leon vl-leon commented Jun 27, 2024

No description provided.

@vl-leon vl-leon requested a review from vobu June 27, 2024 15:26
@vl-leon vl-leon changed the title add pressTile, searchFor add "new book" scenario Jun 28, 2024
@vl-leon vl-leon changed the title add "new book" scenario add create,delete book scenario Jul 1, 2024
lib/index.js Outdated
@@ -0,0 +1,143 @@
async function goHome() {
await browser.goTo('#Shell-home');
await browser.waitForUI5();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where are goTo and waitForUI5() injected into the browser object?
a comment here might help the inclined developer :)
plus: if this is primarily targeting wdi5, the wdi5.goTo() seems more appropriate.
plus: we should refrain from any manual waitForXX APIs and rather leave it to the test framework to sync app lifecyle with test lifecycle. E.g. with wdi5, no waiting for any ops to finish is necessary as the underlying RecordReplay-API takes care of this and is accounted for in wdi5

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switched to wdi5.goTo()

lib/index.js Outdated
}).enterText(text)
}

async function performStandardAction( name ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whitespacing btw method and parenthesis → please introduce a formatter (e.g. prettier) and optionally a linter (e.g. eslint) for avoiding style issues.
I'd suggest to align with wdi5 with that: https://github.com/ui5-community/wdi5/blob/main/.prettierrc

Copy link
Collaborator Author

@vl-leon vl-leon Jul 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added prettier as suggested, eslint also

lib/index.js Outdated
}

async function _findTableWithTitle(title, controlType, opt={}) {
let { searchOpenDialogs } = opt;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure about your intended usage of let and const here and this file in general. Personally, I prefer const over let for immutable var assignments, but that's just me. Please just be aware of it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed that: let it be const ;)

lib/index.js Outdated

async function selectRowInTable( tableTitle, targetRow ) {
let table = await _findTableWithTitle(tableTitle, "sap.m.Table");
let items = await table.getItems();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this has proven to be a potential drawback: if the table in question has growing turned off and at the same time many entries, the test will get flaky here with looong runtimes. Don't have an immediate suggestion for mitigation, just be aware and eventually leave a comment here à la TODO or REVISIT

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a comment

lib/index.js Outdated
let table = await _findTableWithTitle(tableTitle, "sap.m.Table");
let items = await table.getItems();
let checkbox = await items[targetRow].getMultiSelectControl();
await checkbox.fireSelect({selected:true});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please refrain from using any fire* methods. When using standalone, they have unintended side effects such as not triggering any other handlers regisitered with the control. Instead, please use press()

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to press - worked fine


it('go home', async () => {
await lib.goHome();
await _sleep(2 * 1000)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure why this is actually required - I get that FE is slow in general (aka resource-intensive), but waiting 2 secs is just too long IMO.
Best try to get rid of any manual wait operations and leave it to the underlying framework such as wdi5 to sync app- and test-runtime

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed as it is not needed

@vl-leon vl-leon changed the title add create,delete book scenario add create, delete book scenario Jul 4, 2024
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.

2 participants