search

testground / sdk-js

Other
1 stars 3 forks source link

feat: add basic browser support #26

Closed GlenDC closed 1 year ago

GlenDC commented 1 year ago

Supersede https://github.com/testground/sdk-js/pull/25, having applies all the previous feedback.

laurentsenta commented 1 year ago

cc @achingbrain if you want to take a look

GlenDC commented 1 year ago

@laurentsenta this one is ready for a re-review! All checks pass now and comments have been applied.

GlenDC commented 1 year ago

Added functionality to be able to move args from host Env to Browser Env, without having to expose the required keys explicitly: https://github.com/testground/sdk-js/pull/26/commits/06df3ce6c9c904d52fa6d081aa1ffd204b5de69a

(required for: https://github.com/testground/testground/pull/1502#discussion_r1010661571)

GlenDC commented 1 year ago

You can see this version of @testground/sdk active and working in an actual node-browser cross/dual testground test plan: https://github.com/testground/testground/pull/1502

GlenDC commented 1 year ago

Thanks for recreating the PR, it looks great now, two last questions before we can merge:

  • [ ] Could we keep the TODO comments if they were not introduced in this PR? I don't have context on these.
  • [ ] Could we go back to the initial logging approach? I'd rather keep it simple & use the code before implementing optimizations, json outputs, etc. That might be related to testground/testground#1355 (comment)
  1. I'll add the TODO comments back. Reason I removed them is because my linter complained about them... Also, it's like 2 years old, IMHO if a TODO is that long in a codebase it really isn't that important and you can get to it i one day you need it, but then it will present itself. I'm just a guest in this codebase however, so I'll simply add them back :)
  2. I would prefer not. The initial logger approach didn't give any useful logs at all. It's equivalent to just logging 'log' as you get no info at all, due to how testground-sdk is using the logger...
laurentsenta commented 1 year ago

(updated description and title and merging, thanks for the PR!)

mxinden commented 1 year ago

Congratulations @GlenDC and @laurentsenta for this milestone!

BigLep commented 1 year ago

Agreed - way to go!