add exec to ui5 controls to boost performance for data retrieval from many ui5 child controls #456

philippthiele · 2023-04-25T15:15:08Z

Note: documentation is still missing, please let me know what you think of the PR and whether something can be improved, before I start documentation efforts. Thanks!

…eval from many ui5 controls

philippthiele · 2023-04-25T15:17:17Z

With this, it is easy to get a lot of data from all the items of a table for example, which otherwise would take a long time when going via each table item directly. Helps in situations like this, which takes a really long time to resolve due to the many browser roundtrips:

vobu · 2023-04-25T16:39:53Z

🚀 you're clearly on a 👨‍💻 spree here @philippthiele 🔋 ¡excellente!
I really dig the idea of iterating over a massive amount of aggregation children of a control w/o any burden of browser-/Node.js-scope change overhead!
Let me think some on the way the evalOnControl() API is surfaced though - will need to form an opinion and would also like to get the take on this from @Siolto, @monavari-lebrecht, @dominikfeininger and @ArnaudBuchholz. Colleagues: discuss, please! 😄

dominikfeininger · 2023-04-27T11:22:22Z

Hi, nice feature. Good Idea !

Maybe a name like executeOnControl of even execute/ exec ala. Node could be a possibility.

I added some performance testing to your test file. Resulting in roughly bisection of execution time. 👍 for that.

philippthiele · 2023-04-27T13:27:17Z

@dominikfeininger let me know which name fits best to your conventions, I will adapt or you can also do yourself, as far as I know I have enabled edits for maintainers 😊

philippthiele · 2023-05-03T07:50:16Z

Hi @vobu @dominikfeininger ,
do you feel except from aligning on the naming there is something else missing I could contribute still?
Shall I add the documentation now? Is it technically fine from your side?
Thanks!

JanWerder · 2023-05-03T10:54:30Z

This would help a lot, would be great if this could be merged. 👍

Siolto

some node version problems rest looks good. @vobu @dominikfeininger we still want to support node 14, right?

Siolto · 2023-05-03T11:15:01Z

examples/ui5-js-app/package.json

@@ -20,7 +20,7 @@
    "test-selenium": "wdio run e2e-test-config/wdio-selenium-service.conf.js"
  },
  "devDependencies": {
-    "@ui5/cli": "^2.14.10",
+    "@ui5/cli": "^3.1.1",


ui5 tooling v3 requires at least node version 16. Currently we still need to support node 14 that's why our tests are failing

agreed, let's put this on a separate PR to both drop node14 and upgrade to ui5-tooling 3

Siolto · 2023-05-03T11:15:45Z

examples/ui5-js-app/package.json

@@ -35,5 +35,9 @@
    "dependencies": [
      "ui5-middleware-simpleproxy"
    ]
+  },


why do wee need these dependencies?

Siolto · 2023-05-03T11:17:01Z

package.json

@@ -74,7 +76,7 @@
    "@types/sinon": "^10.0.13",
    "@typescript-eslint/eslint-plugin": "^5.42.1",
    "@typescript-eslint/parser": "^5.42.1",
-    "@ui5/cli": "^2.14.14",
+    "@ui5/cli": "^3.1.1",


same here we still test wdi5 with node 14 as well

Siolto · 2023-05-03T11:22:14Z

Hi @philippthiele,

great work and nice idea! Looks like this could boost up test execution quite a lot. From my point of view technically this looks fine. Just some minor remarks from my side. Documentation, fixing the tests and naming alignment is open I guess.

vobu · 2023-05-03T11:57:21Z

examples/ui5-js-app/webapp/test/e2e/evalOnControl.test.js

+                viewName: "test.Sample.view.Main"
+            }
+        })
+        const buttonText = await button.evalOnControl(function () {


what do you think of dropping the function() and return part of evalOnControl() to look "cleaner" similar to this:

await button.evalOnControl( getText() ) // or await button.evalOnControl( this.getText() ) // or await button.evalOnControl( button.getText() )

vobu · 2023-05-03T11:58:48Z

examples/ui5-js-app/webapp/test/e2e/evalOnControl.test.js

+        // *********
+        // new approach -> takes ~4.3sec
+        marky.mark("evalOnControlForListItemTitles")
+        const peopleListNames = await list.evalOnControl(function () {


similar to my suggestion above:

await list.evalOnControl( getItems().map( item => item.getTitle() )

this would even allow to dynamically change the return type, à la

// have an object returned with an embedded array await list.evalOnControl( { getItems().map( item => item.getTitle() } )

vobu · 2023-05-03T12:01:16Z

examples/ui5-js-app/webapp/test/e2e/evalOnControl.test.js

+            }
+        }
+        const peopleListData = await browser.asControl(listSelector).evalOnControl(function () {
+            return {


just for completenes' sake:

browser.asControl(listSelector).evalOnControl( { tableTitle: getHeaderText(), peopleListNames: getItems().map((item) => item.getTitle()) } )

vobu · 2023-05-03T12:02:33Z

src/lib/wdi5-control.ts

@@ -500,6 +500,15 @@ export class WDI5Control {
        }
        // returns the array of [0: "status", 1: result]

+        //special case for evalOnControl, passed function needs to be converted to string to be passed to the browser
+        if (methodName === "evalOnControl") {
+            if (args[0] && typeof args[0] === "function") {


this would of course then have to change when we change the API

vobu · 2023-05-03T12:12:50Z

Here are my .02€:

👀 I left some comments on the way the API looks - let's try and reduce it to the bare minimum. Mind you, my suggestions are suggestions only → open to any kind of mods that make evalOnControl() look "cleaner".

📛 That having said, I'd vouch to change the method name to exec(), as @dominikfeininger already suggested. Looks more in line with the intent of running logic on the control, similar to Node's child_process.exec().

✍️ Additionally, and as @Siolto has pointed out, we'd need documentation on this please 😸

again emphasizing that this is a great improvement! thanks for the work you put into this so far!

philippthiele · 2023-05-04T11:52:28Z

@vobu
Regarding dropping the function:

I would not drop it completely, but make both options available. In a function I might do much more than just access the functions of the control, options are limitless.
I technically do not see how to implement the suggestion though, if I pass it like suggested it will just be executed directly and not on browser side. Can you elaborate a bit? Then I will be happy to implement it.

Rename method to exec: done

Documentation: Will do when everything is clarified and implemented 😊

vobu · 2023-05-04T12:35:17Z

I would not drop it completely, but make both options available. In a function I might do much more than just access the functions of the control, options are limitless.

ah, of course, didn't have this on the radar!
exec( function () { /** stuff */ }) allows to run stuff not limited to the retrieved Control → leave it as is then of course!
btw: could you please add a test to make sure that a lambda/arrow fn à la exec( () => { /** stuff */ }) would also work?

Rename method to exec: done

👍

Documentation: Will do when everything is clarified and implemented 😊

👍

please also don't forget to downgrade ui5-tooling to v2 to re-add Node 14 support

philippthiele · 2023-05-05T07:28:19Z

@vobu I have update the chromedriver in the package lock which fixed a few tests, rest does not seem to be related to a package or the changes in this PR though. Can you help check what is wrong?

Also: I have extended the functionality to accepts arguments as well, as we were talking about anything being possible with a function, passing additional arguments makes sense I would think 😊

Reminder for me: I should check whether propagating errors is possible, to make debugging easier when a browser side error occurs, like calling a non-existent function like "button.getTex()"

vobu · 2023-05-05T09:08:28Z

@vobu I have update the chromedriver in the package lock which fixed a few tests, rest does not seem to be related to a package or the changes in this PR though. Can you help check what is wrong?

sure, I'll peak and see what I can fix. Let me get back to this as soon as I can.

Also: I have extended the functionality to accepts arguments as well, as we were talking about anything being possible with a function, passing additional arguments makes sense I would think 😊

¡excellente!

Reminder for me: I should check whether propagating errors is possible, to make debugging easier when a browser side error occurs, like calling a non-existent function like "button.getTex()"

absolutely, nothing beats DevX improvements.

philippthiele · 2023-05-05T09:59:31Z

@vobu
Errors are now logged to console, couldn't really verify the log in the test, only manually visually. Verified that the return value is null in this case:
dddb3b2

Next step will be documentation.

philippthiele · 2023-05-05T12:29:39Z

@vobu documentation is added 😊 Finishing line is in sight.
Only auth test failing, I can't see any relation to the PR.

Please let me know whether something is missing/can be improved!

dominikfeininger · 2023-05-05T13:31:42Z

Might be worth to check if a general support of direct injected code into browser scope find it's users.

        const fn = () => {
            // since the RecordReplay API etc. is available a full blown test execution would fit in here
            return sap.ui.getCore().byId("container-Sample---Main--Title::NoAction.h1").getText()
        }

        // this code is in wdi5 and could be provided via eg. `browser.execUI5()`
        const result = await browser.executeAsync(function (fn, done) {
            done(eval(fn)())
        }, fn.toString())

        expect(result).toEqual("UI5 demo")

-> next PR

vobu · 2023-05-08T07:57:09Z

@vobu documentation is added 😊 Finishing line is in sight. Only auth test failing, I can't see any relation to the PR.

w00t! I'll re-review asap, also pinging @Siolto and @dominikfeininger to glance over.
and wrt to the failing auth tests, I'll take this as a reminder-to-self to move them to browserstack asap. Seems this is an issue with the gh pipeline as client originator. Sorry for that pesky nagger 🐛

vobu

this. is. so. cool.
there's a small typo (suggested a change).
and i left a hint how to optional test for error messages relayed from browser- to Node-scope, as we have this in a test already.
I'm stoked to see this go in once it has the +1 from Dominik and Simon

vobu · 2023-05-08T08:06:51Z

examples/ui5-js-app/webapp/test/e2e/exec.test.js

+
+
+    it("should be able to propagate a browserside error", async () => {
+        //Log Output during this test should be 3 times: [wdi5] call of exec failed because of: TypeError: this.getTex is not a function


optional enhancement: you can validate error messages on the console in the Node.js scope with the help of sinon.
See

wdi5/examples/ui5-js-app/webapp/test/e2e/error-logging.test.js

Line 26 in 16d6984

const sandbox = sinon.createSandbox()

vobu · 2023-05-08T08:08:19Z

examples/ui5-js-app/webapp/test/e2e/exec.test.js

+        expect(buttonTextArrow2).toEqual(regularBtnText)
+    })
+
+    it("execute function browserside on button to get its text with fluent sync api, basic return type", async () => {


small typo

Suggested change

it("execute function browserside on button to get its text with fluent sync api, basic return type", async () => {

it("execute function browserside on button to get its text with fluent async api, basic return type", async () => {

vobu · 2023-05-08T08:12:08Z

this. is. so. cool.
there's a small typo (suggested a change).
and i left a hint how to optional test for error messages relayed from browser- to Node-scope, as we have this in a test already.
I'm stoked to see this go in once it has the +1 from Dominik and Simon

vobu · 2023-05-09T08:47:13Z

@philippthiele we'll of course announce the new version with this excellent feature, but would you also care to blog about it on blogs.sap.com? Pretty please? 😸

philippthiele · 2023-05-09T09:33:22Z

Thanks to you all for reviewing and merging 😊

@vobu Here are the suggested changes for the test, thanks for that, learned something new: #466
I will try to write a blog post this week, will be on vacation the next, so it could be time-wise hard to manage. Will let you know by the end of the week.

philippthiele · 2023-05-12T10:26:09Z

@vobu I will not be able to produce an adequate blog post before my vacation, I will be happy to pick up the topic afterwards 😊

vobu · 2023-05-12T13:37:55Z

all good! enjoy your time off and recharge properly! looking forward to reading your blog post after your vacation 😄

add evalOnControl to ui5 controls to boost performance for data retri…

837294e

…eval from many ui5 controls

philippthiele changed the title ~~add evalOnControl to ui5 controls to boost performance for data retri…~~ add evalOnControl to ui5 controls to boost performance for data retrieval from many ui5 controls Apr 25, 2023

philippthiele added 2 commits April 25, 2023 17:26

also test object return type for evalOnControl function

cf8cc09

Update evalOnControl.test.js

4434b25

philippthiele changed the title ~~add evalOnControl to ui5 controls to boost performance for data retrieval from many ui5 controls~~ add evalOnControl to ui5 controls to boost performance for data retrieval from many ui5 child controls Apr 25, 2023

feat: add runtime measure

4dc898b

fix(core): run npm audit fix --force

18bbccc

Siolto requested changes May 3, 2023

View reviewed changes

vobu reviewed May 3, 2023

View reviewed changes

rename evalOnControl to exec

e9f9345

Merge branch 'main' into add-evalOnControl-to-ui5-controls

585c9fa

philippthiele changed the title ~~add evalOnControl to ui5 controls to boost performance for data retrieval from many ui5 child controls~~ add exec to ui5 controls to boost performance for data retrieval from many ui5 child controls May 4, 2023

philith and others added 3 commits May 4, 2023 16:52

revert changes to package json and lock files

f4fb8e1

exec: accept arguments & make arrow function work

ad67ec2

update chromedriver in package lock

d117f62

exec: propagate browserside errors to be logged in console

dddb3b2

philippthiele and others added 2 commits May 5, 2023 12:44

Add documentation for exec

bd7cfb3

Merge branch 'main' into add-evalOnControl-to-ui5-controls

7c388a9

vobu approved these changes May 8, 2023

View reviewed changes

dominikfeininger approved these changes May 8, 2023

View reviewed changes

Siolto approved these changes May 9, 2023

View reviewed changes

vobu merged commit 93116d4 into ui5-community:main May 9, 2023

philippthiele deleted the add-evalOnControl-to-ui5-controls branch May 9, 2023 09:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add exec to ui5 controls to boost performance for data retrieval from many ui5 child controls #456

add exec to ui5 controls to boost performance for data retrieval from many ui5 child controls #456

philippthiele commented Apr 25, 2023 •

edited

Loading

philippthiele commented Apr 25, 2023 •

edited

Loading

vobu commented Apr 25, 2023

dominikfeininger commented Apr 27, 2023

philippthiele commented Apr 27, 2023

philippthiele commented May 3, 2023 •

edited

Loading

JanWerder commented May 3, 2023

Siolto left a comment

Siolto May 3, 2023

vobu May 3, 2023

Siolto May 3, 2023

Siolto May 3, 2023

Siolto commented May 3, 2023

vobu May 3, 2023

vobu May 3, 2023

vobu May 3, 2023

vobu May 3, 2023

vobu May 3, 2023

vobu commented May 3, 2023 •

edited

Loading

philippthiele commented May 4, 2023

vobu commented May 4, 2023

philippthiele commented May 5, 2023 •

edited

Loading

vobu commented May 5, 2023

philippthiele commented May 5, 2023

philippthiele commented May 5, 2023

dominikfeininger commented May 5, 2023 •

edited

Loading

vobu commented May 8, 2023

vobu left a comment

vobu May 8, 2023

vobu May 8, 2023

vobu commented May 8, 2023

vobu commented May 9, 2023

philippthiele commented May 9, 2023

philippthiele commented May 12, 2023 •

edited

Loading

vobu commented May 12, 2023



		it("should be able to propagate a browserside error", async () => {
		//Log Output during this test should be 3 times: [wdi5] call of exec failed because of: TypeError: this.getTex is not a function

	it("execute function browserside on button to get its text with fluent sync api, basic return type", async () => {
	it("execute function browserside on button to get its text with fluent async api, basic return type", async () => {

add exec to ui5 controls to boost performance for data retrieval from many ui5 child controls #456

add exec to ui5 controls to boost performance for data retrieval from many ui5 child controls #456

Conversation

philippthiele commented Apr 25, 2023 • edited Loading

philippthiele commented Apr 25, 2023 • edited Loading

vobu commented Apr 25, 2023

dominikfeininger commented Apr 27, 2023

philippthiele commented Apr 27, 2023

philippthiele commented May 3, 2023 • edited Loading

JanWerder commented May 3, 2023

Siolto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siolto commented May 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vobu commented May 3, 2023 • edited Loading

philippthiele commented May 4, 2023

vobu commented May 4, 2023

philippthiele commented May 5, 2023 • edited Loading

vobu commented May 5, 2023

philippthiele commented May 5, 2023

philippthiele commented May 5, 2023

dominikfeininger commented May 5, 2023 • edited Loading

vobu commented May 8, 2023

vobu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vobu commented May 8, 2023

vobu commented May 9, 2023

philippthiele commented May 9, 2023

philippthiele commented May 12, 2023 • edited Loading

vobu commented May 12, 2023

philippthiele commented Apr 25, 2023 •

edited

Loading

philippthiele commented Apr 25, 2023 •

edited

Loading

philippthiele commented May 3, 2023 •

edited

Loading

vobu commented May 3, 2023 •

edited

Loading

philippthiele commented May 5, 2023 •

edited

Loading

dominikfeininger commented May 5, 2023 •

edited

Loading

philippthiele commented May 12, 2023 •

edited

Loading