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

fix methods for the multiremote #6761

Closed
wants to merge 2 commits into from
Closed

Conversation

elaichenkov
Copy link
Member

Proposed changes

bugfix: #6760

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)

@christian-bromann
Copy link
Member

Thanks for raising the PR 👏

I suggest to add a smoke test to https://github.com/webdriverio/webdriverio/blob/main/tests/multiremote/test.js where a command is called via browser.<browserName>.<command>() so we can ensure it works properly. You can read more about our smoke tests in the contribution guidelines. Let me know if you have any questions.

@elaichenkov
Copy link
Member Author

I've just pushed one test, but again I'm not sure that's what we actually need for that case :(
So, feel free to modify it :)

@christian-bromann
Copy link
Member

@elaichenkov the smoke test the way you put it makes sense. However when I reviewed the change as a whole it turns out that it already got fixed by this patch: #6759. Can you double check? In this case I would suggest to not merge given that adding writeable is not really necessary.

@elaichenkov
Copy link
Member Author

I've just tested it with the patch you've mentioned, but unfortunately, the problem still exists with the same message: we cannot assign it to read-only property. After adding the writable property the issue seems to be disappeared.

@christian-bromann
Copy link
Member

@elaichenkov you are right, the only problem is that the smoke test also passes without the change. So running the example using the example wdio script fails but running the multiremote smoke test passes just fine.

@christian-bromann
Copy link
Member

A bit of investigation showed that _NOT_FIBER is where the error is thrown in the passing case undefined and in the failing case a [Function: wrapCommandFn]

@elaichenkov
Copy link
Member Author

@elaichenkov you are right, the only problem is that the smoke test also passes without the change. So running the example using the example wdio script fails but running the multiremote smoke test passes just fine.

Totally agree with you.

A bit of investigation showed that _NOT_FIBER is where the error is thrown in the passing case undefined and in the failing case a [Function: wrapCommandFn]

Well, sorry for the silly question but what does it mean for us?

@christian-bromann
Copy link
Member

Well, sorry for the silly question but what does it mean for us?

Sorry for not being clear, I did a console.log(this._NOT_FIBER) right before the error is thrown and when running with the example files (where it fails) I get [Function: wrapCommandFn] and when running using the smoke tests (and where it passes) I get undefined. This confuses me because both things run with the same setup (wdio testrunner + @wdio/sync). While I am not too much concerned about this it would be great if the multiremote test could help us validate this fix.

@elaichenkov
Copy link
Member Author

Oh, I see.
Thank you for explaining.
I'll try to explore it and conduct a root cause.

@konstantin-gorbachev-Simpligov

New updates. Also when I try use await browser.url("/") in async mode. Have the error: Cannot assign to read only property '_NOT_FIBER' of object '#'

@christian-bromann
Copy link
Member

What is the status of this? Anything I can do to help move this forward?

@elaichenkov
Copy link
Member Author

Sorry, I haven't gotten a chance to take a look at this yet.

@christian-bromann
Copy link
Member

Closing due to inactivity. I believe this is fixed now.

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.

None yet

3 participants