-
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
v0.47.0 release notes for browser #3356
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.
@ankur22 the branch for release notes is https://github.com/grafana/k6/tree/release-notes-v0.47.0. You have to use it as the base branch.
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## release-notes-v0.47.0 #3356 +/- ##
========================================================
Coverage ? 72.95%
========================================================
Files ? 261
Lines ? 20018
Branches ? 0
========================================================
Hits ? 14604
Misses ? 4487
Partials ? 927
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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 for helping me 🙇 Some small suggestions.
Are we missing #1010: Fix FrameSession.executionContextForID and #1009: Add Page.executionContextForID?
@inancgumus I was waiting for a response on this comment. Added it now in 6b447e1. Good spot though! 😄 |
19d0898
to
1d0f96c
Compare
By the way, we need reverse this syntax as it doesn't look like a link:
Into:
Do you think we should also add an example here? @ka3de WDYT? |
This was fixed already in
We should have discussed that in its own PR 😄 We can add it here, but for me the release notes is a small announcement on what is new, just a small introduction, adding examples to every feature/change might make it too long. If we still want to add an example, would this make sense to you? page.on('console', msg => {
check(msg, {
'assertConsoleMessageType': msg => msg.type() == 'log',
'assertConsoleMessageText': msg => msg.text() == 'this is a console.log message 42',
'assertConsoleMessageArgs0': msg => msg.args()[0].jsonValue() == 'this is a console.log message',
'assertConsoleMessageArgs1': msg => msg.args()[1].jsonValue() == 42,
});
});
page.evaluate(() => console.log('this is a console.log message', 42)); BTW should we d try to add comments associated to source code changes, so we can resolve the conversations and they do not pollute so much the PR |
Co-authored-by: İnanç Gümüş <[email protected]>
Co-authored-by: İnanç Gümüş <[email protected]>
Co-authored-by: Oleg Bespalov <[email protected]>
38c4723
to
b761f40
Compare
What?
This adds the changes to the
v1.1.0
tagged browser module to the release notes. It doesn't add the changes forcookies
andpage.on
since they are being added by two other PRs.Why?
We need to update our users of what changes we have made to the browser module.
Checklist
make ci-like-lint
) and all checks pass.make tests
) and all tests pass. (NA)Related PR(s)/Issue(s)
Related: #3353 & #3352.