-
Notifications
You must be signed in to change notification settings - Fork 60
Conversation
…ce for wdio-cucumber-framework
Cucumber2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial suggestions and questions
@@ -14,7 +14,7 @@ The easiest way is to keep `wdio-cucumber-framework` as a devDependency in your | |||
```json | |||
{ | |||
"devDependencies": { | |||
"wdio-cucumber-framework": "~0.2.0" | |||
"wdio-cucumber-framework": "~1.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should change line 8 to:
A WebdriverIO plugin. Adapter for CucumberJS v2 testing framework. To use v1 install this package with latest version
v0.3.1
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea
circle.yml
Outdated
test: | ||
override: | ||
- npm run test:ci | ||
- ./node_modules/.bin/codeclimate-test-reporter < ./coverage/lcov.info |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We used to use TravisCI for CI/CD .. if circle ci is free for open source stuff I don't mind staying with circle ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since it's in this repo now it automatically works with travis, I personally prefer circle so I was going to use it in mine - I can remove this file, I'm assuming you use travis everywhere else so it doesn't make sense to break the pattern
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming you use travis everywhere else
that is the case
lib/adapter.js
Outdated
// native | ||
string.match(/^async /) || | ||
// babel (this may change, but hey...) | ||
string.match(/return _ref[^.]*\.apply/) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we really want to do that. I think it is sophisticated enough when we check if the function name is async
or the config is sync: false
. If the user wants to use async/await we should expect him to turn sync off in the config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it didn't work for me originally.. but I tried it now with just the fn.name === 'async' check and it passes the tests. Unless I'm missing some edge case I forgot about - you might be right and that check might be enough, removed this code now
@@ -1,7 +1,7 @@ | |||
{ | |||
"name": "wdio-cucumber-framework", | |||
"version": "0.3.1", | |||
"description": "A WebdriverIO plugin. Adapter for Cucumber testing framework.", | |||
"version": "1.0.0-beta.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If all existing tests have passed I am cool to just release it as 1.0.0
. There will always be bug fixes and improvements.
Edit: unless the next release takes too long than we can release it as beta with the fork as dependency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense
package.json
Outdated
@@ -39,7 +39,8 @@ | |||
"homepage": "https://github.com/webdriverio/wdio-cucumber-framework#readme", | |||
"dependencies": { | |||
"babel-runtime": "~6.23.0", | |||
"cucumber": "~1.3.1", | |||
"cucumber": "lgandecki/cucumber-js#passOptionsToFunctionWrapperNPM", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen the pending PR here: cucumber/cucumber-js#838
Let's wait for the v1.0.0 release until this is merged any published or maybe release it as beta and use the forked version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure :)
this uses the oficial cucumber2 now, I fixed tags as well (the syntax has changed between v1 and v2). |
what we've been using now is to have one "entry" file that we load in require section of the wdio.conf.js file, that loads everything else. But that doesn't seem optimal. |
Can't wait for this to get merged. |
@@ -37,74 +41,83 @@ class CucumberAdapter { | |||
this.capabilities = capabilities | |||
|
|||
this.cucumberOpts = Object.assign(DEFAULT_OPTS, config.cucumberOpts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// convert require path to files
this.cucumberOpts.require = this.cucumberOpts.require.map(function (path) {
if (path.match(/.*\*\*\/\*.js/)) return glob.sync(path, {cwd: process.cwd()})
if (path.match(/.*\.js/)) return path
return glob.sync(path + '/**/*.js', {cwd: process.cwd()})
}).reduce(function (a, b) {
return a.concat(b)
}, [])
You can add something like this after this line.
This is to parse the cucumber require option. Please also add glob to import, and dependency in package.json.
By the way, is there a reason we are not using Cucumber.Cli directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the tip @pantherqin and for the reminder that this is not merged yet! I'm about to try your suggestion and let you know how this goes.
as for CLI, per this comment:
cucumber/cucumber-js#786 (comment)
and also, I did start with CLI at first but I encountered some limitations that seemed almost impossible to solve (I can't recall right now, it was hundreds if not thousands of lines of code ago ;) ). Maybe there wasn't a way to attach custom listeners?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya, understand it was not merged yet. I may want to use webdriverIO with cucumber 2 for a company project (pending boss approval :D).
Was using protractor with cucumber. You can have a look at https://github.com/protractor-cucumber-framework/protractor-cucumber-framework/blob/master/index.js if you want. I didn't read the code in too much detail yet, but it seem to work well using cucumber Cli for protractor with other hooks. Hope it helps :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is a question for @christian-bromann
Do we want to allow globs in the require part of the wdio.conf?
Also, we could use similar code (with glob) to automatically import files from step_definitions/ folder, the way cucumber (and current wdio-cucumber-framework implementation) works, as far as I understand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to allow globs in the require part of the wdio.conf?
If it is complaint to the behavior of Cucumber, then yes
Disables "replacing existing mock" warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! Even though this has some outstanding questions, let's merge this and glo forward. Thanks so much @lgandecki
Cucumber.js already released v3 🙈 |
Feedback/questions more than welcome!