feat(mocha): add test runner wrapper + remove redundant code #11
Conversation
|
You should change the name of the PR... it is a refactor now (; |
|
Sorry, I actually see you added some features, so not a refactor, feature/s... |
|
yes, currently i didn't break the existing API. |
mastertheblaster
changed the title
| const theEyes = new Eyes(); | ||
|
|
||
| theEyes.setOs(process.platform); | ||
| theEyes.setApiKey(process.env.EYES_API_KEY); |
There was a problem hiding this comment.
If I get it right, this code will run when you require eyes.js, regardless you have api key or not, which might end with an error, in case you do not have an api key.
There was a problem hiding this comment.
I have already checked that manually. It allows to pass '', undefined, null value anything.
There was a problem hiding this comment.
The main question about this PR is that it changes the approach.
It's more like original eyes.it, but with less coupling and maybe more readable.
There was a problem hiding this comment.
It's not matcher anymore
There was a problem hiding this comment.
yes... we might consider maybe to move this to a different package, or change package name. I think that a different package will be cleaner. WDYT?
There was a problem hiding this comment.
ok, will move to package with-eyes
There was a problem hiding this comment.
I think that we will want to have at least an option for batching with the matcher (I didn't have time to think about it, yet). WDYT?
| @@ -1,22 +1,44 @@ | |||
| const [, , , chaiToMatchScreenshot, browserWSEndpoint] = process.argv; | |||
| const {Assertion, expect} = require('chai'); | |||
| const {withEyes} = require('../../mocha'); | |||
There was a problem hiding this comment.
We're running this test in a different process and passing other links to require via arguments. Not sure this is the best solution but we might want to be aligned with the same method for all requires? (no matter which we choose)
No description provided.