percy / percy-agent

[Deprecated in favor of `@percy/cli`] An agent process for integrating with Percy.
https://percy.io
MIT License
22 stars 25 forks source link

Javascript error: unsafe member access services/agent-service.js #515

Open Dmitry9 opened 4 years ago

Dmitry9 commented 4 years ago

There is a javascript error during execution of the services/agent-service.js.

It happens when request.body.domSnapshot is not a typeof Array or String.

Screenshot from 2020-05-28 13-01-20 Please can this line: https://github.com/percy/percy-agent/blob/master/src/services/agent-service.ts#L82

if (domSnapshotLog.length > Constants.MAX_LOG_LENGTH) {

be changed to something like:

if (domSnapshotLog && domSnapshotLog.length > constants_1.default.MAX_LOG_LENGTH) {

Same goes for: https://github.com/percy/percy-agent/blob/master/src/services/agent-service.ts#L120

if (domSnapshot.length > constants_1.default.MAX_FILE_SIZE_BYTES) {

to

if (domSnapshot && domSnapshot.length > constants_1.default.MAX_FILE_SIZE_BYTES) {

@djones djones FYI

Robdel12 commented 4 years ago

Hm, my first question here is why is the domSnapshot empty? That's a much larger issue 🤔

happens when request.body.domSnapshot is not a typeof Array.

FWIW, we're expecting it to be a string here.

wwilsman commented 4 years ago

A result of this issue could be to add an error when domSnapshot is falsey so it's a bit more clear that it's a required property. I could see the possibility that domSnapshot might be empty or misspelled when starting to create a custom SDK.

Dmitry9 commented 4 years ago

Hm, my first question here is why is the domSnapshot empty? That's a much larger issue

happens when request.body.domSnapshot is not a typeof Array.

FWIW, we're expecting it to be a string here.

@Robdel12 Sorry for not verified info is not a typeof Array. It happens inside CircleCi container so debugging is a bit harder. I updated issue description.

Robdel12 commented 4 years ago

No worries! If domSnapshot is not present, nothing with Percy will work. I'm curious if you could explain a little more about what's going on/what you're doing?

Dmitry9 commented 4 years ago

@Robdel12 I am starting percy with nightwatch:

"test:e2e": "percy exec -- nightwatch -c .circleci/nightwatch.js -e chrome,firefox;",

it connects to selenium/hub -> chrome and firefox processes it does everything as expected if I manually (with nodejs script replace those two lines in respective to https://github.com/percy/percy-agent/blob/master/src/services/agent-service.ts js file). So to avoid this dirty hack with nodejs script I ask to resolve this issue. Thank you.

alubchuk commented 4 years ago

@Robdel12 @wwilsman hey guys, I also see this issue while running Percy with nightwatch for firefox. Fix that @Dmitry9 proposed prevents percy to fail and stop executing tests. What are your thoughts on it? Do you still need more info?

Robdel12 commented 4 years ago

If you're getting this error, I'm very curious to know why or what is causing the domSnapshot string to be null. If that's null, exiting out of that statement will only result in issues later in the process.

alubchuk commented 4 years ago

@Robdel12 Ok, I will try to collect more informative log of what's happening and will share it with you. By now it only happened while running tests in firefox.

Robdel12 commented 4 years ago

Are there any errors in the Firefox browser console? Seems like there might be a bug in that version of Firefox + the DOM cloning we're doing. That could possibly return null 🤔

alubchuk commented 4 years ago

@Robdel12 After some additional testing I figured out that this error is introduced not only in firefox, but in chrome as well. I turned on nightwatch logging so here you can check what's going on:

image

image

In @percy/nightwatch: image

alubchuk commented 4 years ago

@Robdel12 I see that @percy/nightwatch@1.1.0 has chromedriver@79.0.0, not chromedriver@84.0.1as a dependency as it is in your master branch on github. Do you plan to release it soon? Probably I'm getting some of these errors because of old chromedriver API.

alubchuk commented 4 years ago

Unfortunately chromedriver doesn't yet fully support execute command: https://chromium.googlesource.com/chromium/src/+/master/docs/chromedriver_status.md