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

Add missing typings #3319

Merged
merged 10 commits into from Jan 15, 2019

Conversation

@ablok
Copy link
Contributor

ablok commented Jan 14, 2019

Proposed changes

  • Adds type DesiredCapabilities[] to the typings and documentation
  • Adds property execArgv to the typings and documentation
  • Adds typings and documentation for the various hooks

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)

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)

Further comments

Please review thoroughly since I did some assumptions.
I added the missing execArgv property but have 3 questions:

  • Did not add a default to a constant file since the default is null (I presume). Is that ok? I apparently also did not add a default for the services property that I added in a previous MR.
  • I made it optional. Is that ok? Looking at the other properties, I am unclear on when it should be optional or not.
  • I put it in the webdriver.tpl.d.ts file and not in the webdriverio.tpl.d.ts file. Is that ok?

Reviewers: @webdriverio/technical-committee

@codecov

This comment has been minimized.

Copy link

codecov bot commented Jan 14, 2019

Codecov Report

Merging #3319 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3319      +/-   ##
=========================================
+ Coverage   96.69%   96.7%   +<.01%     
=========================================
  Files         131     131              
  Lines        2846    2850       +4     
  Branches      613     616       +3     
=========================================
+ Hits         2752    2756       +4     
  Misses         87      87              
  Partials        7       7
Impacted Files Coverage Δ
packages/webdriverio/src/constants.js 91.66% <ø> (ø) ⬆️
packages/wdio-config/src/constants.js 100% <ø> (ø) ⬆️
packages/webdriver/src/monad.js 100% <0%> (ø) ⬆️
packages/webdriverio/src/commands/browser/$.js 100% <0%> (ø) ⬆️
packages/webdriverio/src/commands/browser/$$.js 95.65% <0%> (ø) ⬆️
packages/wdio-allure-reporter/src/index.js 98.43% <0%> (ø) ⬆️
packages/webdriverio/src/commands/element/$$.js 95.83% <0%> (+0.18%) ⬆️
packages/webdriverio/src/commands/element/$.js 95.23% <0%> (+0.23%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fdc7c15...1468efe. Read the comment docs.

Copy link
Member

christian-bromann left a comment

One comment

* Gets executed when an error happens, good place to take a screenshot.
* @param {String} error message
*/
onError: function(message) {

This comment has been minimized.

Copy link
@christian-bromann

christian-bromann Jan 14, 2019

Member

This hook was not ported and should be removed.

This comment has been minimized.

Copy link
@ablok

ablok Jan 14, 2019

Author Contributor

Removed it from everywhere.

@ablok

This comment has been minimized.

Copy link
Contributor Author

ablok commented Jan 14, 2019

Can you please tell me:

  • How do I determines if a property can be optional in the config file?
  • What determines if there needs to be a default specified in the constants file.
  • How do I determine if a property in the config (like services and execArgv) is part of webdriver or webdriverio?
Copy link
Member

christian-bromann left a comment

One last comment

docs/Options.md Outdated Show resolved Hide resolved
@christian-bromann

This comment has been minimized.

Copy link
Member

christian-bromann commented Jan 14, 2019

  • How do I determines if a property can be optional in the config file?

There is no logic yet that checks if a config option is required or not. It could be a useful thing even though I had a different opinion once webdriverio-boneyard/v5#22

  • What determines if there needs to be a default specified in the constants file.

Nothing determines if some option needs a default value. It is currently just a suggestion in the constant files.

  • How do I determine if a property in the config (like services and execArgv) is part of webdriver or webdriverio?

All options described for the webdriver package are also valid for webdriverio. Only if you use webdriverio with the wdio testrunner (@wdio/cli) there are more options necessary.


Type: `Object`<br>
Default: `{ browserName: 'firefox' }`<br>
Type: `Object`|`Object[]`<br>

This comment has been minimized.

Copy link
@christian-bromann

christian-bromann Jan 14, 2019

Member

As I mentioned, we should move this to WDIO options and keep Object as type. Under WDIO options we can put capabilities and mention to have it set as Object (for multiremote) or Object[]

This comment has been minimized.

Copy link
@christian-bromann

christian-bromann Jan 14, 2019

Member

to clarify,
WebDriver Options has a capabilities option that has type Object
WDIO Options has also a capabilities option from type Object|Object[] , first when run as multiremote

ablok added 3 commits Jan 14, 2019
Copy link
Member

christian-bromann left a comment

Even tho the smoke tests fail I will merge this because the problem within the smoke test is fixable. Thanks for the changes!

@christian-bromann christian-bromann merged commit 365b516 into webdriverio:master Jan 15, 2019
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
licence/cla Contributor License Agreement is signed.
Details
@ablok

This comment has been minimized.

Copy link
Contributor Author

ablok commented Jan 15, 2019

I just pushed a fix :)

@christian-bromann

This comment has been minimized.

Copy link
Member

christian-bromann commented Jan 15, 2019

yamkay pushed a commit to MoveInc/webdriverio that referenced this pull request Jun 13, 2019
## Proposed changes

- Adds type `DesiredCapabilities[]` to the typings and documentation
- Adds property `execArgv` to the typings and documentation
- Adds typings and documentation for the various hooks

[//]: # (Describe the big picture of your changes here to communicate to the maintainers why we should accept this pull request. If it fixes a bug or resolves a feature request, be sure to link to that issue.)

## Types of changes

[//]: # (What types of changes does your code introduce to WebdriverIO?)
[//]: # (_Put an `x` in the boxes that apply_)

- [x] 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)

## Checklist

[//]: # (_Put an `x` in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code._)

- [x] I have read the [CONTRIBUTING](https://github.com/webdriverio/webdriverio/blob/master/CONTRIBUTING.md) doc
- [ ] I have added tests that prove my fix is effective or that my feature works
- [x] I have added necessary documentation (if appropriate)

## Further comments

[//]: # (If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...)

Please review thoroughly since I did some assumptions.
I added the missing `execArgv` property but have 3 questions:
- Did not add a default to a constant file since the default is null (I presume). Is that ok? I apparently also did not add a default for the `services` property that I added in a previous MR.
- I made it optional. Is that ok? Looking at the other properties, I am unclear on when it should be optional or not.
- I put it in the `webdriver.tpl.d.ts` file and not in the `webdriverio.tpl.d.ts` file. Is that ok?

### Reviewers: @webdriverio/technical-committee
yamkay pushed a commit to MoveInc/webdriverio that referenced this pull request Sep 4, 2019
## Proposed changes

- Adds type `DesiredCapabilities[]` to the typings and documentation
- Adds property `execArgv` to the typings and documentation
- Adds typings and documentation for the various hooks

[//]: # (Describe the big picture of your changes here to communicate to the maintainers why we should accept this pull request. If it fixes a bug or resolves a feature request, be sure to link to that issue.)

## Types of changes

[//]: # (What types of changes does your code introduce to WebdriverIO?)
[//]: # (_Put an `x` in the boxes that apply_)

- [x] 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)

## Checklist

[//]: # (_Put an `x` in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code._)

- [x] I have read the [CONTRIBUTING](https://github.com/webdriverio/webdriverio/blob/master/CONTRIBUTING.md) doc
- [ ] I have added tests that prove my fix is effective or that my feature works
- [x] I have added necessary documentation (if appropriate)

## Further comments

[//]: # (If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...)

Please review thoroughly since I did some assumptions.
I added the missing `execArgv` property but have 3 questions:
- Did not add a default to a constant file since the default is null (I presume). Is that ok? I apparently also did not add a default for the `services` property that I added in a previous MR.
- I made it optional. Is that ok? Looking at the other properties, I am unclear on when it should be optional or not.
- I put it in the `webdriver.tpl.d.ts` file and not in the `webdriverio.tpl.d.ts` file. Is that ok?

### Reviewers: @webdriverio/technical-committee
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.