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

Add waitForEvent tasks to release notes #3413

Merged
merged 1 commit into from
Nov 2, 2023

Conversation

ankur22
Copy link
Contributor

@ankur22 ankur22 commented Oct 23, 2023

What?

Update the next release notes with links to the PRs that have fixed and refactored code to get waitForEvent to work.

Why?

To update the release notes for the next release.

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make lint) and all checks pass.
  • I have run tests locally (make tests) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

@github-actions github-actions bot requested review from mstoykov and oleiade October 23, 2023 15:41
@ankur22 ankur22 changed the title Add/taskqueue fix Add waitForEvent tasks to release notes Oct 23, 2023
@ankur22 ankur22 requested review from inancgumus and ka3de October 23, 2023 15:42
@ankur22 ankur22 changed the base branch from master to release-notes-v0.48.0 October 23, 2023 15:43
@codecov-commenter
Copy link

codecov-commenter commented Oct 23, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (release-notes-v0.48.0@af4dd63). Click here to learn what that means.

❗ Current head 5fa24ab differs from pull request most recent head 09ebc51. Consider uploading reports for the commit 09ebc51 to get more accurate results

Additional details and impacted files
@@                   Coverage Diff                    @@
##             release-notes-v0.48.0    #3413   +/-   ##
========================================================
  Coverage                         ?   73.11%           
========================================================
  Files                            ?      258           
  Lines                            ?    19650           
  Branches                         ?        0           
========================================================
  Hits                             ?    14367           
  Misses                           ?     4401           
  Partials                         ?      882           
Flag Coverage Δ
ubuntu 73.05% <0.00%> (?)
windows 72.95% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -33,8 +33,7 @@ _Format as `<number> <present_verb> <object>. <credit>`_:

## Bug fixes

_Format as `<number> <present_verb> <object>. <credit>`_:
- _`#111` fixes race condition in runtime_
- [browser#1042](https://github.com/grafana/xk6-browser/pull/1042), [browser#1069](https://github.com/grafana/xk6-browser/pull/1069) Fixes `browserContext.waitForEvent`. This fix involved promisifying the `waitForEvent` API as well as various internal refactors.
Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding is that grafana/xk6-browser#1069 is not specifically related with waitForEvent, as it also refactors the usage of TaskQueue for Page.on. Should we remove it from here and instead add an entry for it under Maintenance and internal improvements section?

Copy link
Contributor Author

@ankur22 ankur22 Oct 30, 2023

Choose a reason for hiding this comment

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

Yeah, good point, it does include quite a bit of refactoring to move the taskqueue out of the common package and into the mapping layer. The main motivation (initially) for this PR though was due to the issue of not working with the taskqueue when using the predicate function for waitForEvent, i.e. without this change waitForEvent wouldn't work. Happy to move it to Maintenance and internal improvements though if you still think it should, WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

To me it feels moving the TaskQueue to the mapping layer is a separate task. But no strong opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, happy with that change added it to a fixup: ef92133

Copy link
Contributor

Choose a reason for hiding this comment

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

How specific should we be with these internal refactors? To be honest I'm never sure of what should be the level of detail in these cases. Considering that this is a completely internal change, I don't disagree with the current text, but I wonder if it should be more specific such as "Internal refactor to move Goja's TaskQueue away from core implementation" (or similar).

Since you are a reviewer for this also, any thoughts @oleiade ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mind if i merge this and we can open a new issue for this if we need?

@ankur22 ankur22 force-pushed the add/taskqueue-fix branch 2 times, most recently from ef92133 to b043560 Compare October 31, 2023 12:22
Copy link
Contributor

@ka3de ka3de left a comment

Choose a reason for hiding this comment

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

LGTM.

@ankur22 ankur22 merged commit dadafb1 into release-notes-v0.48.0 Nov 2, 2023
20 checks passed
@ankur22 ankur22 deleted the add/taskqueue-fix branch November 2, 2023 11:29
@mstoykov mstoykov added this to the v0.48.0 milestone Nov 10, 2023
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.

5 participants