Issue #11132 - adding hookName to beforeHook and afterHook by HananArgov · Pull Request #11148 · webdriverio/webdriverio

HananArgov · 2023-09-11T23:22:27Z

Proposed changes

Adding hookName to beforeHook and afterHook per issue #11132

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

as seen in the issue, this will able us to see which hook is referred in beforeHook and afterHook

Reviewers: @webdriverio/project-committers

erwinheitzman

Nice going 👍

erwinheitzman · 2023-09-11T23:37:49Z

packages/wdio-utils/src/test-framework/testFnWrapper.ts

        string,
-        number
+        number,
+        string?


From what I can tell, this isn't optional and is always passed

i'm leaving this optional, since runSpec in testInterfaceWrapper calls it as well, and then we'll add an unused argument (origFn.name is it)

erwinheitzman · 2023-09-11T23:38:58Z

packages/wdio-utils/src/test-framework/testFnWrapper.ts

 ) {
    const retries = { attempts: 0, limit: repeatTest }
    const beforeArgs = beforeFnArgs(this)
+    if (type !== 'Test') {


Let's explicitly add it to the functions we want instead of adding it to everything but Test

i'm not sure if i understood what you meant, or know how implement.

do you mean that i'll need to remove the if, so it would work with type==='Test' as well? or that when we push to this to afterArgs/beforeArgs the hookName will be inserted as well, as part of line 70.

if it's the second one, i'm a bit stumped here, since i can't quite understand how to do this (help wanted 🙂)

changed the comparison to look for Hook, since we want to implement this on all hooks

packages/wdio-utils/src/test-framework/testFnWrapper.ts

erwinheitzman

Left another comment.

We also have to update the docs and templates with the new argument

fix according to comment Co-authored-by: Erwin Heitzman <15839059+erwinheitzman@users.noreply.github.com>

HananArgov · 2023-09-12T13:49:50Z

#11148 (review)
i've updated the templates and documents, except for the i18n, since I don't know the other languages

christian-bromann

It would be nice to have a test for this to ensure we verify this functionality!

packages/wdio-utils/src/test-framework/testFnWrapper.ts

christian-bromann

One comment on the position of the argument in the afterHook interface. Also, I still would like to see a smoke test that verifies this feature exist.

christian-bromann · 2023-09-12T23:12:58Z

packages/wdio-types/src/Services.ts

-     */
-    afterHook?(test: Test, context: any, result: TestResult): unknown | Promise<unknown>
+    */
+    afterHook?(test: Test, context: any, hookName: string, result: TestResult): unknown | Promise<unknown>


This could be a potentially breaking change for user that access the result object, how about we also add it as last argument?

i'm trying to do so, but i'm getting an compiling issue with this part:

any idea on what to do here?

const emitHookEvent = ( fnName: string, eventType: string ) => ( _test: never, _context: never, { error }: { error?: jasmine.FailedExpectation } = {}, _hookName: never

Just leave out the _hookName parameter

Small reminder to update the docs/templates since the argument is now last

packages/wdio-utils/src/test-framework/testFnWrapper.ts

erwinheitzman · 2023-09-19T23:55:15Z

Ping @HananArgov

HananArgov · 2023-09-21T12:02:38Z

sorry, i've been on vacation abroad for a while, and came back today.
i've done the changes, please review them again, hopefully I didn't miss anything

HananArgov · 2023-09-21T13:21:51Z

any idea on why the tests failed? i'm not sure if they are related to the changes i've made

christian-bromann · 2023-09-21T19:05:10Z

@HananArgov it seems that the coverage dropped, feel free to update the branch coverage to 88%

packages/wdio-browserstack-service/src/service.ts

examples/wdio/vite-vue-example/wdio.conf.ts

website/i18n/bg/docusaurus-plugin-content-docs/current/Configuration.md

HananArgov · 2023-09-25T20:20:58Z

@christian-bromann i'm now getting this issue:

packages/wdio-jasmine-framework/src/index.ts(159,33): error TS2345: Argument of type '(_test: never, _context: never, { error }?: { error?: jasmine.FailedExpectation | undefined; }) => void' is not assignable to parameter of type '(test: any, context: any, hookName: string) => unknown'.
  Types of parameters '__2' and 'hookName' are incompatible.
    Type 'string' has no properties in common with type '{ error?: FailedExpectation | undefined; }'.

I can't understand what to do here.
is there a chance maybe to arrange a video meeting to try and solve this?
I think I went way over my knowledge here

christian-bromann · 2023-09-29T07:00:36Z

is there a chance maybe to arrange a video meeting to try and solve this?

Sure, please feel free to block a slot in the Open Office Hours.

christian-bromann

Nice work, LGTM 👍

Issue webdriverio#11132 - adding hookName to beforeHook and afterHook

7bd851c

erwinheitzman reviewed Sep 11, 2023

View reviewed changes

change according to comments

87a7a30

HananArgov requested a review from erwinheitzman September 12, 2023 09:59

webdriverio deleted a comment Sep 12, 2023

erwinheitzman reviewed Sep 12, 2023

View reviewed changes

packages/wdio-utils/src/test-framework/testFnWrapper.ts Outdated Show resolved Hide resolved

erwinheitzman requested changes Sep 12, 2023

View reviewed changes

HananArgov and others added 3 commits September 12, 2023 16:25

Update packages/wdio-utils/src/test-framework/testFnWrapper.ts

78546b4

fix according to comment Co-authored-by: Erwin Heitzman <15839059+erwinheitzman@users.noreply.github.com>

updating templates and docs

e15421f

Merge branch 'main' into master

2adea38

HananArgov requested a review from erwinheitzman September 12, 2023 13:50

fix typing issues

a38f6ad

christian-bromann reviewed Sep 12, 2023

View reviewed changes

packages/wdio-utils/src/test-framework/testFnWrapper.ts Outdated Show resolved Hide resolved

packages/wdio-utils/src/test-framework/testFnWrapper.ts Outdated Show resolved Hide resolved

fixes according to comments + types

80484b2

HananArgov requested a review from christian-bromann September 12, 2023 16:55

christian-bromann reviewed Sep 12, 2023

View reviewed changes

HananArgov added 2 commits September 21, 2023 11:50

moving hookName to be the last

d508588

Merge branch 'main' into master

bbd69c3

HananArgov requested a review from christian-bromann September 21, 2023 12:02

Merge branch 'main' into master

6ca80a9

christian-bromann reviewed Sep 23, 2023

View reviewed changes

packages/wdio-browserstack-service/src/service.ts Outdated Show resolved Hide resolved

examples/wdio/vite-vue-example/wdio.conf.ts Show resolved Hide resolved

website/i18n/bg/docusaurus-plugin-content-docs/current/Configuration.md Show resolved Hide resolved

fix according to comments

201672d

HananArgov requested a review from christian-bromann September 29, 2023 06:15

christian-bromann and others added 3 commits October 4, 2023 00:10

Merge branch 'main' into master

26a18db

fixed compilation error and tests

096c7e7

update coverage

0b513ec

christian-bromann approved these changes Oct 4, 2023

View reviewed changes

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

christian-bromann merged commit b314ac1 into webdriverio:main Oct 4, 2023

Uh oh!

Conversation

HananArgov commented Sep 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Types of changes

Checklist

Further comments

Reviewers: @webdriverio/project-committers

Uh oh!

erwinheitzman 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!

Uh oh!

erwinheitzman left a comment

Choose a reason for hiding this comment

Uh oh!

HananArgov commented Sep 12, 2023

Uh oh!

christian-bromann left a comment

Choose a reason for hiding this comment

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!

christian-bromann Sep 13, 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!

Uh oh!

erwinheitzman commented Sep 19, 2023

Uh oh!

HananArgov commented Sep 21, 2023

Uh oh!

HananArgov commented Sep 21, 2023

Uh oh!

christian-bromann commented Sep 21, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

HananArgov commented Sep 25, 2023

Uh oh!

christian-bromann commented Sep 29, 2023

Uh oh!

christian-bromann left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

HananArgov commented Sep 11, 2023 •

edited

Loading

christian-bromann Sep 13, 2023 •

edited

Loading