Move taskqueue to mapping layer #1069
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ankur22
force-pushed
the
poc/tq-to-mapping
branch
8 times, most recently
from
be41b64
to
f3bda45
Compare
ankur22
commented
Oct 17, 2023
ankur22
force-pushed
the
poc/tq-to-mapping
branch
from
f3bda45
to
a64579c
Compare
ka3de
reviewed
Oct 18, 2023
There was a problem hiding this comment.
Thank you for working on this @ankur22 !
Overall I think it looks good, it's true that we are adding complexity to the mapping layer, which ideally it's not desired, but it's a tradeoff in order to simplify our core logic, so I think it's probably a good step forward.
ankur22
force-pushed
the
poc/tq-to-mapping
branch
from
d422aba
to
1e7d7b7
Compare
3 tasks
ankur22
force-pushed
the
poc/tq-to-mapping
branch
from
833dfde
to
91651bb
Compare
inancgumus
previously approved these changes
Oct 19, 2023
ankur22
force-pushed
the
poc/tq-to-mapping
branch
from
6855e30
to
b90ee66
Compare
Removes the goja TaskQueue from the page implementation and instead places it in the mapping layer by wrapping the handler in a function that will execute it in the local TaskQueue.
It's no longer required to return an error from the handler, as this is handled in the TaskQueue wrapper in the mapping layer. This change also refactors the test so that: 1. it no longer returns an error from the handler; 2. the event loop has been removed since the taskqueue has been removed from the business logic, making the test simpler to understand with less orchestration.
With this change we only use the TaskQueue from the main thread, as it's required to avoid race conditions with goja. Unfortunately this implies an explicit close call for the TaskQueue that we are wrapping in the page.close() call in the mapping layer.
To enable us to work with the task queue outside of the business logic and allow the setup of the predicate function within the task queue we need to: 1. Parse the options in the mapping layer. 2. Create a func that can call the predicate function from within a taskqueue. 3. We need to wait for the taskqueue call to complete and return the response back to the caller. 4. Change the browserContext API within the go code.
This refactors the tests so that it matches the new signature for waitForEvent, which is that it takes in a predicate method and a timeout. The predicate method will now be wrapped and added to a taskqueue in the mapping layer which means we do not need to work with the event loop in the test itself. Since the options are now parsed in the mapping layer, this also simplifies how we pass the predicate and timeout arguments to waitForEvent.
This is just cleaning up the arguments so that they are grouped in a better way, as well as shortening the line length to avoid the linter issue.
Since waitForEvent is a promisified function, we need to make the taskqueue thread safe.
The taskqueue wasn't working when it was one per vu since it became difficult to understand exactly when it was best to close the taskqueue if there were multiple pages. This change now creates a taskqueue per page, but the taskqueues is stored in the vu rather than in the page struct itself, which helps ensure that the taskqueue is kept away from the page business logic.
This will be used to register, retrieve and close the taskqueues which is required by the page.on and browserContext.waitForEvent APIs.
Since a require.True will FailNow in the current goroutine and not the root goroutine of the test, it's best to pass an error back to assert on in the test's root goroutine.
ankur22
force-pushed
the
poc/tq-to-mapping
branch
from
b90ee66
to
468d84c
Compare
This was referenced Oct 23, 2023
5 tasks
5 tasks
inancgumus
added
productivity
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
This PR moves the taskqueue to the mapping layer. The change will affect
page.onandbrowserContext.waitForEvent.Why?
As we work more with goja and it's runtime, it is starting to find its way into our business logic code. This make maintaining the project more difficult due to having to work with raw
goja.Valueand (more recently) the event loop andtaskqueue. One of these areas that we do not want in the business logic is most definitely thetaskqueuewhich was introduced when we addedpage.on.page.onrequires a predicate method on the goja runtime since the function is written in JS. Since the goja runtime is not thread safe we need to work with thetaskqueue. Working with thetaskqueuein the business logic makes reasoning, maintaining and writing tests very tricky since various components need to be orchestrated in multiple goroutines.This change forces the creation and deletion of the
taskqueuein the mapping layer. This change also forces us to convertgoja.Values to native go structs, which has enabled us to move some goja code out of the business logic (specifically forbrowsercontext.waitForEvent). We can verify that the changes work with JS test scripts, whereas the business logic can be tested with native go code using either unit or integration tests with less go routine orchestration.Checklist
Related PR(s)/Issue(s)