search

percy / cli

The Percy CLI is used to interact with, and upload snapshots to, percy.io via the command line.
https://docs.percy.io/docs/cli-overview
71 stars 45 forks source link

Percy reports "Invalid snapshot options:" even when there is zero problems #1674

Closed navrkald closed 3 months ago

navrkald commented 3 months ago

The problem

Percy reports "Invalid snapshot options:" even when there is zero problems.

Environment

Debug logs

tsc && PERCY_BRANCH=staging npx percy exec -c censored.js -- node censored.js [percy] Notice: Percy collects CI logs for service improvement, stored for 30 days. Opt-out anytime with export PERCY_CLIENT_ERROR_LOGS=false [percy] Percy has started! [percy] Running "node censored.js" [percy] Invalid snapshot options: [percy] Snapshot taken: censored [percy] Finalized build censored [percy] Build's CLI and CI logs sent successfully. Please share this log ID with Percy team in case of any issues - 35435194_build_35435194_cli_5bdfdd0532339ace71a37854a9c60702339fe9f6 [percy] Command "node censored.js" exited with status: 0

davidrunger commented 3 months ago

@navrkald : I believe that you are seeing this because of what I mentioned here: https://github.com/percy/cli/pull/1656#issuecomment-2240277498 . A fix to address the reason that error started emerging is here: https://github.com/percy/cli/pull/1669 .

That being said, from the PR that you have put together (https://github.com/percy/cli/pull/1675), I notice that you have noticed something else relevant, which is that [percy] Invalid snapshot options: will be logged even if errors is an empty array, since an empty array is truthy in JavaScript. Prior to the issue that I mentioned above, errors was undefined, which is why nothing was logged, but now errors is an empty array, which is why [percy] Invalid snapshot options: is logged, but then no further details about what is invalid are logged.

Your PR might or might not be something that the Percy maintainers want to accept. In some sense, it's an improvement, since it would avoid this behavior of logging [percy] Invalid snapshot options: but then logging no details about what is invalid. But, in another sense, maybe it's arguably slightly worse, since it might mask underlying problems. In other words, in this case, where adding cookies caused a validation error, should that error have been surfaced to the user? In other words, maybe rather than checking that the errors array has a non-zero length before we log [percy] Invalid snapshot options:, instead the focus should be on making sure that the cookies-related validation errors that triggered this actually make it into the errors array that is checked in the code that you are touching in that PR.

Or maybe the Percy maintainers want to implement both your change (checking that errors is a non-empty array) and what I mentioned (populating these cookies-related errors (or other similar errors in the future) into the errors array).

navrkald commented 3 months ago

Hi @davidrunger I am afraid expecting empty errors array as false is clearly bug which should be fixed asap. If someone doesn't know/believe empty array is truth expression he can run this snipped in browser JS console and will quickly see that my PR is just fixing this super obvious and super simple bug.

if([]) {
  console.log("Empty array is truthfully expression")
}

I am afraid cookie issue, which I just quickly checked, is unrelated issue.

sarah-gelt commented 3 months ago

This is most definitely a bug - if the errors array is empty then it shouldn't be outputting anything. I just upgraded to 1.29 and thought something must be wrong in our tests even though we have made no other changes related to Percy. I ran one of our smallest projects (only ~25 tests), the output is a mess and it sent me scrambling to the documentation to work out if there's something I missed about the new version that meant our code was now incompatible. Our largest suite has almost 200 tests in it, I can't imagine what that output will look like next time we run it.

Having found this issue report, I now know there is nothing wrong our end and I can warn the rest of our team to ignore all these messages until it's sorted.

image
sarah-gelt commented 3 months ago

@navrkald your PR got approved 🙌 Hopefully this will get into a release soon.