Map k6 browser API to Goja

[inancgumus]

As discussed in issue #661, we're now "manually" mapping the browser module methods to Goja. The global Goja field name mappers are removed from the code. This can be automated in the future as the mapping can be error-prone.

The current mappers are just forwarders to the inner methods, and we add $ and $$ for the Query and QueryAll methods.

Let me know if I missed something while mapping the methods; and whether the way we solved it is good enough.

Closes #661.

[mstoykov]

I have left some comments, but looks good in general.

I would not be able to guess if that is enough to not break anything, but I would argue (again) that you should have more JS code tests that use the API as users will do.

This will also help with making certain that this mapping are what you want - as it will exercise them ;)

[ankur22]

It feels like there are lot of things that could go wrong (i'm not saying they have) i.e. the mappings could be incorrect. Is there anyway we can test the objects that are now manually being created and mapped?

[inancgumus]

@ankur22, thanks for your review 🙇

It feels like there are lot of things that could go wrong (i'm not saying they have) i.e. the mappings could be incorrect. Is there anyway we can test the objects that are now manually being created and mapped?

Are we having to mapRequest and mapResponse because it can return a frame, which in turn can return pages? Seems like this is going to catch people out as we will forget to add new methods to the mappings. I preferred the approach where we looped through all the methods and just set them to the object, like above in mapBrowserToGoja.

mapBrowserToGoja is the root mapping, that's why it has a loop. We automatically add the other mappings thanks to the root mapping, so they don't need a loop. The main concern of Mihail was magically mapping the methods, so this PR is about manually doing it and locking in the behavior with tests.

I agree that the solution we have in place is not future-proof, but it's good enough for now. In the future, we should consider automating or generating this mapping code.

I've added some tests to verify the behavior, which you can find in the following commits:

• d062048
• 0c388d5

On the bright side, users will receive an error message if something goes wrong and a method can't be found. With the tests in place, the likelihood of that happening should be low. Since the tests test whether all API methods (api/) is in the mappings or not. We should add more JS tests, though.

Additionally, these manual mappings will allow us to move the goja layer from our domain logic code, making it more type-safe and easier to work with. I'll create an issue about that soon.

Let me know if you have any questions or concerns.

---

Related:

The PR description:

As discussed in the issue #661, we're now "manually" mapping the browser module methods to Goja. The global Goja field name mappers are removed from the code. This can be automated in the future as the mapping can be error-prone.

Also here:

However, not sure the current solution will be future-proof. Since we have a relatively large API surface area; it can be error-prone to manually do this kind of mappings. I believe we might want to automate this in the future (maybe by writing a source-code generator). That way the resulting mapping can be in the code itself and people can easily track them without magic.

[inancgumus]

I have left some comments, but looks good in general.

@mstoykov, thank you for your review 🙇

I would not be able to guess if that is enough to not break anything, but I would argue (again) that you should have more JS code tests that use the API as users will do.
This will also help with making certain that this mapping are what you want - as it will exercise them ;)

We now have tests to check whether we map the entire API surface that have the wildcard methods. I agree we should have more JS tests, preferably in the examples/ folder. I believe we should work on this next.

I'm also thinking of changing the directory name to jstest or something like that. Since they won't be examples anymore and we already have examples in the k6-browser docs.

[ankur22]

LGTM. I've left some minor comments.

Having looked at the unit tests you've added in this, i'm not sure adding JS tests would add any more benefit to this, I could be wrong though.

[mstoykov]

It feels like there are lot of things that could go wrong (i'm not saying they have) i.e. the mappings could be incorrect. Is there anyway we can test the objects that are now manually being created and mapped?

Arguably having them "manually" written (and hopefully tested manually soon as well) makes it less likely that something will just break because the "magic" translation layer changes or someone unknowingly renames a method which was used.

Additioanlly apart from $ and $$ I would argue that having fields such as page.request is way easier and more explicit with having a goja.Object created and defined "manually" then it being "magically" created by goja.

[inancgumus]

LGTM. I've left some minor comments.

Thank you for the re-review @ankur22.

Having looked at the unit tests you've added in this, i'm not sure adding JS tests would add any more benefit to this, I could be wrong though.

JS tests can ensure that we are mapping the methods correctly to the ones we mapped. For example, what would happen if we map request.frame to something else? Since request.frame now returns a *goja.Object, we won't detect whether we correctly mapped to a Frame. We could add more Go tests, but it would be like re-implementing everything in tests.

Also, JStests are necessary for the rest of the extension too. So we can exercise them from the point of view of a user without setting up a testing suite from the Go side with all these goja translations.

These tests can also allow us to refactor the extension more freely instead of changing the Go code. So we can extract goja out of the extension and let the goja translation layer do the job (see #271).