Skip to content
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

Auto capture and insert cookie in asset discovery browser #1656

Merged
merged 8 commits into from
Jul 19, 2024

Conversation

chinmay-browserstack
Copy link
Contributor

@chinmay-browserstack chinmay-browserstack commented Jul 10, 2024

  • Capture cookies using document.cookie in serialize-dom so can be used in asset discovery.
  • Use this cookie and insert in asset discovery browser.
  • Disable this behaviour using PERCY_DO_NOT_USE_CAPTURED_COOKIES env variable
  • Prioritise cookie passed via percy.yml over auto captured cookie.

Note: httponly cookie can't be captured so that needs to be passed using percy.yml

Copy link
Contributor

@rishigupta1599 rishigupta1599 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@chinmay-browserstack chinmay-browserstack merged commit 9181d68 into master Jul 19, 2024
36 checks passed
@chinmay-browserstack chinmay-browserstack deleted the auto_capture_cookies branch July 19, 2024 06:55
@davidrunger
Copy link

davidrunger commented Jul 19, 2024

@chinmay-browserstack @prklm10 I think that this PR is causing me to see the following warning in my logs:

[percy] Invalid snapshot options:

There is no additional logging about what the specific invalid snapshot options are; only that log line is printed.

Example here: https://github.com/davidrunger/david_runger/actions/runs/10013824950/job/27682261695#step:10:231

Part of the reason that I believe that this PR is the cause of that warning being logged is because if I open this file on my local machine -- node_modules/.pnpm/node_modules/@percy/config/dist/validate.js -- and if I add the following to the top of the validate function that is defined therein on L186, then I stop seeing the log line about [percy] Invalid snapshot options: when I run my test:

delete data.domSnapshot.cookies;

I tried this because when (before making the just-mentioned addition) I logged ajv.errors, part of the output included this:

    // ...
    keyword: 'unevaluatedProperties',
    params: { unevaluatedProperty: 'cookies' },
    message: 'must NOT have unevaluated properties',
    // ...

@davidrunger
Copy link

Also, downgrading from @percy/cli 1.29.0 to 1.28.9 fixes the issue (i.e. causes [percy] Invalid snapshot options: not to be logged anymore).

@chinmay-browserstack
Copy link
Contributor Author

@davidrunger Thanks for pointing this out. i have checked and found cookies field is missing from config as you mentioned.

Created a fix PR for it #1669. it will be fixed in next release.
Meanwhile you can continue using @percy/cli 1.29.0 as it just prints warning and has no impact on any functionality of percy cli.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants