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

Fixes #6786 #6896

Merged
merged 4 commits into from
May 25, 2021
Merged

Fixes #6786 #6896

merged 4 commits into from
May 25, 2021

Conversation

jonn-set
Copy link
Contributor

Proposed changes

Fixes #6786

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

Its a simple one line fix on session reload,

Reviewers: @webdriverio/project-committers

Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

Can we have a test for this? I would advocate for a unit test.

Comment on lines 44 to 45
if (this.puppeteer)
this.puppeteer = undefined;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (this.puppeteer)
this.puppeteer = undefined;
if (this.puppeteer) {
delete this.puppeteer
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That does not work for some reason. I get this error on the 2nd browser.url call, first one goes through without any issues.

image

This is a sample project I am running by linking the package.

Stacktrace:

[0-0] Error in "1: I search for 'reloadSession'"
invalid session id: No active session with ID 78bdf25b0b241b231f6b3fec05a7b5b5
    at Object.getErrorFromResponseBody (C:\Users\jonne\workspace\node.js\wdio-jonn\packages\webdriver\src\utils.ts:221:12)
    at WebDriverRequest._request (C:\Users\jonne\workspace\node.js\wdio-jonn\packages\webdriver\src\request.ts:195:23)
    at processTicksAndRejections (internal/process/task_queues.js:95:5)
    at Browser.runCommandWithHooks (C:\Users\jonne\workspace\node.js\wdio-jonn\packages\wdio-sync\src\wrapCommand.ts:125:25)
    at Browser.runCommandWithHooks (C:\Users\jonne\workspace\node.js\wdio-jonn\packages\wdio-sync\src\wrapCommand.ts:118:24)
    at Browser.wrapCommandFn (C:\Users\jonne\workspace\node.js\wdio-jonn\packages\wdio-sync\src\wrapCommand.ts:78:44)
    at World.<anonymous> (C:\Users\jonne\workspace\node.js\browser.mock-defect\features\step-definitions\steps.js:7:13)
    at World.executeSync (C:\Users\jonne\workspace\node.js\browser.mock-defect\node_modules\@wdio\sync\build\index.js:38:22)
    at C:\Users\jonne\workspace\node.js\browser.mock-defect\node_modules\@wdio\sync\build\index.js:69:68

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@christian-bromann What next, do you want to try something else?

@jonn-set
Copy link
Contributor Author

@christian-bromann I have changed it to check for puppeteer to be connected instead of the variable to exist. Check if you are happy with it please?

@jonn-set
Copy link
Contributor Author

@christian-bromann added unit tests to increase coverage, also fixed the broken mocks, let me know if you happy with the overall solution & test coverage please?

@jonn-set
Copy link
Contributor Author

@christian-bromann bump

Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

👍 Thanks a lot!

@christian-bromann christian-bromann added the PR: Bug Fix 🐛 PRs that contain bug fixes label May 25, 2021
@christian-bromann christian-bromann merged commit c72e8be into webdriverio:main May 25, 2021
@jonn-set
Copy link
Contributor Author

@christian-bromann Thanks for merging. Can I ask when's the next release, please?

@christian-bromann
Copy link
Member

It was suppose to happen already but the pipeline failed. Looking into it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Bug Fix 🐛 PRs that contain bug fixes
Projects
None yet
2 participants