Refactor parsing of arguments for waitForFunction #1183
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
This was referenced Jan 23, 2024
ankur22
commented
Jan 23, 2024
| @@ -457,6 +465,39 @@ func mapFrame(vu moduleVU, f *common.Frame) mapping { | |||
| return maps | |||
| } | |||
|
|
|||
| func parseWaitForFunctionArgs( | |||
There was a problem hiding this comment.
Not sure about this. Should i just copy and paste the parsing instead of trying to abstract them out into its own function?
There was a problem hiding this comment.
I prefer to keep it this way, grouped in a separate function, instead of having it scattered around the mappings implementation. Also considering that this is called more than once, for mapPage and mapFrame.
ankur22
commented
Jan 23, 2024
ankur22
force-pushed
the
refactor/waitForFunction
branch
4 times, most recently
from
8eae5b8 to
df6c88f
Compare
ankur22
force-pushed
the
refactor/page-click-parse
branch
from
7875f68 to
3b92c86
Compare
ankur22
force-pushed
the
refactor/waitForFunction
branch
from
df6c88f to
b32530b
Compare
ankur22
force-pushed
the
refactor/page-click-parse
branch
2 times, most recently
from
9cbdd2f to
d57dba3
Compare
ankur22
force-pushed
the
refactor/waitForFunction
branch
from
b32530b to
ec39914
Compare
Refactor the parsing of the optional options out of the frame.waitForFunction promise, and move it into the mapping layer which is on the main goja thread and out of the promise. This will mitigate the risk of accessing the non thread safe goja runtime from more than one goroutine. This change also adds a temporary change in page.waitForFunction to parse the goja value, which will also change at a later stage.
Move the transformation of the goja callable to a string outside of the promise for frame.waitForFunction. This is to avoid using the non thread safe goja runtime from multiple goroutines. This change also includes a temporary change to page.waitForFunction which will still do the transformation in the promise for now.
This moves the transformation of the args for the frame.waitForFunction API to the mapping layer and out of the promise. This will help mitigate the risk of using the non thread safe goja runtime from multiple goroutines, and only work with it from the main goja thread in the mapping layer. This adds a temporary change to page.waitForFunction where it still does the transform within the promise. This will be moved at a later stage.
This method will be used by both frame and page types to parse the argument for their respective waitForFunction methods.
Use the new parseWaitForFunctionArgs in frame.waitForFunction.
This function is quite useful elsewhere that requires running async js scripts.
This new function is to only be used by integration tests when needing to test the mapping layer along with the core business logic for page.
This refactors the parsing of the arguments out of the promise and into the mapping layer, just as we have done for frame.waitForFunction. This will help mitigate any issues where we try to access the goja runtime from multiple goroutines which is not thread safe.
ankur22
force-pushed
the
refactor/waitForFunction
branch
from
ec39914 to
52c8211
Compare
This function will be used by TestPageWaitForFunction. That test requires access to the mapped versions of page. Instead of exporting a new MapPage method in mapping.go, work with the existing method to create a new module instance, set the mapped browser to the runtime and work with the event system.
This is a custom function for TestWaitForFunction to allow it to run the async API in an event loop.
The test now works with startIteration and runJSInEventLoop.
This is no longer needed since TestWaitForFunction now works with the existing workflow on mapping the desired values.
This change was made to allow TestWaitForFunction to work with the same method to run js code in an eventloop, but it now uses its own method.
ka3de
reviewed
Jan 24, 2024
mstoykov
reviewed
Jan 24, 2024
Add the test runtime to the k6test.VU type. This is useful when needing to run async js functions on the goja runtime's event loop within a test.
This refactors the waitForFunction tests to use the test runtime which already has a built in helper method to run js on the goja runtime's event loop.
inancgumus
reviewed
Jan 25, 2024
ka3de
approved these changes
Jan 25, 2024
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?
Refactor the parsing of the arguments of
waitForFunctionAPIs so that they are done outside the promise and therefore on the main goja thread.Why?
This will help mitigate issues which can occur when trying to access the goja runtime from multiple goroutines, which can causes panics since the goja runtime is not thread safe.
Checklist
Related PR(s)/Issue(s)
Updates: #1180 & #1181