shared store resource pool #10010 by pedrorfernandes · Pull Request #10029 · webdriverio/webdriverio

pedrorfernandes · 2023-03-22T12:17:13Z

Solves #10010

Proposed changes

Types of changes

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update

Checklist

I have read the CONTRIBUTING doc
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)
I have added proper type definitions for new commands (if appropriate)

Further comments

Reviewers: @webdriverio/project-committers

pedrorfernandes · 2023-03-22T12:20:18Z

@christian-bromann I will create docs + tests for this once the API is approved

I had to modify the setInterval code because it would be difficult to re-use it with setResourcePool, please validate if this approach is ok.

To not break contract with setValue, I had to return an empty promise when baseUrl is not set yet. Kind of confusing but I didn't test if using await setValue() in onPrepare could dead lock the hook (opted just to not break contract instead)

christian-bromann

Some comments. After we agree on the implementation, let's make sure to add some unit tests and docs.

packages/wdio-shared-store-service/src/server.ts

packages/wdio-shared-store-service/src/client.ts

packages/wdio-shared-store-service/src/server.ts

christian-bromann

One comment but the implementation looks good to me. Let's add some tests for it.

christian-bromann · 2023-03-23T17:34:54Z

packages/wdio-shared-store-service/src/service.ts

-import { getValue, setValue, setPort } from './client.js'
+import { getValue, setValue, setPort, setResourcePool, getValueFromPool, addValueToPool } from './client.js'
 import type { SharedStoreServiceCapabilities } from './types.js'
+import type { JsonArray } from '@wdio/types'


We already import types in line 1, let's merge these.

oops, added now
will work on tests tomorrow

pedrorfernandes · 2023-03-28T14:54:21Z

packages/wdio-shared-store-service/src/server.ts

+            resourcePoolStore.set(key, value)
+            return res.end()
+        })
+        .post('/getValueFromPool/:key', async (req, res, next) => {


@christian-bromann setting this to .get caused me issues, unit tests that throw errors would hang for 2s instead of an immediate reply. .post() throws the error immediately

Let's fix the unit test then, this should definitely be a get method. Given it is not really a user facing API I am happy to ignore the url path.

figured it out, got has a retry mechanism on error, I've set all throwHttpErrors: false, retry: { limit: 0 }

also, to note: GET operations should not modify the state of data, that's why got was retrying, assuming that it wouldn't make a difference in the server. A POST operation is more suited to our api call because it's actually modifying internal data

pedrorfernandes · 2023-03-28T14:54:51Z

packages/wdio-shared-store-service/src/server.ts

+            const key = req.params.key as string
+
+            if (!resourcePoolStore.has(key)) {
+                next(new Error(`'${key}' resource pool does not exist. Set it first using 'setResourcePool'`))


@christian-bromann I had to change throwing to next(new Error, this is the correct way for polka to throw error messages

sounds good, let's do:

Suggested change

next(new Error(`'${key}' resource pool does not exist. Set it first using 'setResourcePool'`))

return next(new Error(`'${key}' resource pool does not exist. Set it first using 'setResourcePool'`))

changed to

return next(`'${key}' resource pool does not exist. Set it first using 'setResourcePool'`)`

because new Error serializes to {}, serializing all errors by hand is too cumbersome
Polka recommends to just next(string) because this throws the error

I've added a catch + throw error on the client to read this message

pedrorfernandes · 2023-03-28T14:59:36Z

packages/wdio-shared-store-service/tests/server.test.ts

+                })
+
+                describe('and the timeout is not specified', () => {
+                    it('should return a value within the default timeout', async () => {


@christian-bromann I couldn't test the max timeout situation

the issue is: got.post needs a free event loop to run it's flush() and dispatch the request

I would need to release the event loop so that got can work. It doesn't expose the flush fn for us to make it work immediately

However, using vi.useFakeTimers() cancels all timers. This means that in the test i can not release the event loop with a setTimeout and take it back later. got can not run this way

I couldn't figure out a way to jump start got without releasing the event loop

also the test for timeout takes

✓ packages/wdio-shared-store-service/tests/server.test.ts (14) 1281ms

doesn't affect the unit test pipeline a lot, but want to know your thoughts

christian-bromann

Some comments, great work so far 👍

packages/wdio-shared-store-service/README.md

christian-bromann · 2023-03-28T19:40:21Z

packages/wdio-shared-store-service/src/server.ts

+            resourcePoolStore.set(key, value)
+            return res.end()
+        })
+        .post('/getValueFromPool/:key', async (req, res, next) => {


Let's fix the unit test then, this should definitely be a get method. Given it is not really a user facing API I am happy to ignore the url path.

christian-bromann · 2023-03-28T19:44:01Z

packages/wdio-shared-store-service/src/server.ts

+            const key = req.params.key as string
+
+            if (!resourcePoolStore.has(key)) {
+                next(new Error(`'${key}' resource pool does not exist. Set it first using 'setResourcePool'`))


sounds good, let's do:

Suggested change

next(new Error(`'${key}' resource pool does not exist. Set it first using 'setResourcePool'`))

return next(new Error(`'${key}' resource pool does not exist. Set it first using 'setResourcePool'`))

packages/wdio-shared-store-service/tests/client.test.ts

packages/wdio-shared-store-service/tests/server.test.ts

pedrorfernandes · 2023-03-30T11:13:40Z

packages/wdio-shared-store-service/src/client.ts

-const errHandler = (err: Response<Error>) => {
-    log.warn(err.statusCode, err.statusMessage, err.url, err.body)
+const errHandler = (err: RequestError) => {
+    throw new Error(`${err.response?.body || 'Shared store server threw an error'}`)


@christian-bromann please validate this

before: the polka server would crash on throw error inside the HTTP handler, and all wdio processes crashed suddenly
now: the polka server returns an error, this client throws it to wdio, the message is logged, the worker crashes, but wdio process continues on to the next test

i think this was the desired behaviour, not to immediately crash all running suites

Sounds good to me 👍

christian-bromann

Thank you for all this work. I want to include this into the next release so I will go ahead, merge this and address the missing pieces separately.

christian-bromann · 2023-04-05T17:21:26Z

packages/wdio-shared-store-service/src/client.ts

-const errHandler = (err: Response<Error>) => {
-    log.warn(err.statusCode, err.statusMessage, err.url, err.body)
+const errHandler = (err: RequestError) => {
+    throw new Error(`${err.response?.body || 'Shared store server threw an error'}`)


Sounds good to me 👍

shared store resource pool #10010

feb68ea

christian-bromann reviewed Mar 22, 2023

View reviewed changes

pedrorfernandes added 2 commits March 22, 2023 16:30

shared store resource pool #10010

470d0ab

shared store resource pool #10010

3cd85d6

christian-bromann reviewed Mar 23, 2023

View reviewed changes

shared store resource pool #10010

3c8edbf

christian-bromann added the PR: New Feature 🚀 PRs that contain new features label Mar 26, 2023

shared store resource pool #10010

2c37c0d

pedrorfernandes commented Mar 28, 2023

View reviewed changes

shared store resource pool #10010

0e42520

christian-bromann requested changes Mar 28, 2023

View reviewed changes

christian-bromann mentioned this pull request Mar 30, 2023

[💡 Feature]: shared-store atomic operations #10010

Closed

1 task

pedrorfernandes added 2 commits March 30, 2023 12:01

shared store resource pool #10010

7b17751

shared store resource pool #10010

510927a

pedrorfernandes commented Mar 30, 2023

View reviewed changes

christian-bromann approved these changes Apr 5, 2023

View reviewed changes

christian-bromann merged commit d87519f into webdriverio:main Apr 5, 2023

christian-bromann mentioned this pull request Apr 5, 2023

Have better internal API for shared store #10121

Merged

8 tasks

	next(new Error(`'${key}' resource pool does not exist. Set it first using 'setResourcePool'`))
	return next(new Error(`'${key}' resource pool does not exist. Set it first using 'setResourcePool'`))

Uh oh!

Conversation

pedrorfernandes commented Mar 22, 2023

Proposed changes

Types of changes

Checklist

Further comments

Reviewers: @webdriverio/project-committers

Uh oh!

pedrorfernandes commented Mar 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

christian-bromann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

christian-bromann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christian-bromann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pedrorfernandes Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christian-bromann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pedrorfernandes commented Mar 22, 2023 •

edited

Loading

pedrorfernandes Mar 30, 2023 •

edited

Loading