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
Wdio Percy Support v8 #11865
Wdio Percy Support v8 #11865
Conversation
Nice PR, left some minor feedback 👍 |
@erwinheitzman I have pushed changes as per the comments above. Can you do another round of review? Thanks! |
@christian-bromann could you also review this one? I think it's good to have two pair of eyes on this one 🙂 |
This is the corresponding PR for WebdriverIO V7 |
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.
Initial set of comments.
return false | ||
} | ||
|
||
// node v0.10 |
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.
What is this comment about?
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.
fs.accessSync was introduced post node 0.10+ so added handling as a failsafe
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.
WebdriverIO defines which Node version it supports and it expects that all interfaces are supported for these versions. I don't see a need to document this in the code. Furthermore, I don't believe anyone uses a Node.js version older than v1.
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.
Removed
@christian-bromann Resolved the above comments, can you do another round of review post these changes? Thanks! |
Looks good from the BrowserStack side. |
This reverts commit 0ba8af2. "removed # prefix for private fields"
review changes
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.
Another set of suggestions
@christian-bromann I have addressed all the comments, can you please check if it looks good |
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.
LGTM 👍 Thanks!
@rev-doshi happy to merge once the unit test pass |
@christian-bromann thank you! I have fixed the UT issue and run them at my end. |
Sorry, merged to quickly. I wanted to say: LGTM 👍 thank you for the contribution! |
Proposed changes
BrowserStack Percy Support to capture screenshots & snapshots on pre-existing test builds and generate reports on Percy.
Types of changes
Checklist
Further comments
Reviewers: @webdriverio/project-committers