-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat: clear and delete cookies [#2476] #2509
feat: clear and delete cookies [#2476] #2509
Conversation
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.
Hey @Maksimall89!
Thanks for your contribution. Changes look good 👍
However, I'd like to propose two small things:
- remove unused
opts goja.Value
argument - cover new functionality with the tests.
Cheers!
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.
Hey @Maksimall89, thanks for your contribution. 🎉 I added a single request, let me know if I'm missing the why you added it.
As @olegbespalov said the test part is also very important for us. 🙏
Codecov Report
@@ Coverage Diff @@
## master #2509 +/- ##
==========================================
+ Coverage 72.71% 75.45% +2.73%
==========================================
Files 184 202 +18
Lines 14571 15998 +1427
==========================================
+ Hits 10596 12071 +1475
+ Misses 3333 3171 -162
- Partials 642 756 +114
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Hey everyone, thanks for your answers! I need time to understand and fix tests |
I fixed it, I hope... |
hey, I add |
@Maksimall89 Unfortunately, right now, adding Regarding the running the tests, you can use a simple |
@olegbespalov thx, I add |
@codebien could you restart tests? I see a flaky test https://github.com/grafana/k6/runs/6320853392?check_suite_focus=true because I nothing changed there |
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, @Maksimall89 thanks again for your updates, we plan to merge this PR during the next week.
Ok, thank you for your support! |
I have a scenario where clear and delete cookies don't work. Probably it is something I don't understand, but do you mind taking a look? |
Realized feature #2476.
clear
all cookies for a particular URLdelete
cookies for a particular URLDemo script:
Output: