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.

Sign up for GitHub

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

Jump to bottom

feat: add basic browser support #26

Merged
merged 13 commits into from
Nov 16, 2022
Merged

feat: add basic browser support #26

merged 13 commits into from
Nov 16, 2022

Conversation

GlenDC
Copy link
Contributor

@GlenDC GlenDC commented Nov 9, 2022 •
edited by laurentsenta
Loading

  • use shims + browser field to support
  • add registerTestcaseResult to communicate end of test with a node runner when running in-browser
  • Expose getEnvParameters to let a client gather and move testground variables (used for browser support).

Supersede #25, having applies all the previous feedback.

@GlenDC GlenDC mentioned this pull request Nov 9, 2022
Closed
@laurentsenta laurentsenta self-requested a review November 10, 2022 09:16
@laurentsenta
Copy link
Contributor

cc @achingbrain if you want to take a look

@GlenDC GlenDC mentioned this pull request Nov 12, 2022
Open
@GlenDC
Copy link
Contributor Author

GlenDC commented Nov 12, 2022

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

GlenDC
GlenDC commented Nov 12, 2022
View reviewed changes
Copy link
Contributor Author

@GlenDC GlenDC left a comment

Choose a reason for hiding this comment

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

LGTM

@GlenDC GlenDC mentioned this pull request Nov 12, 2022
Merged
3 tasks
@GlenDC
Copy link
Contributor Author

GlenDC commented Nov 12, 2022 •
edited
Loading

Added functionality to be able to move args from host Env to Browser Env, without having to expose the required keys explicitly: 06df3ce

(required for: testground/testground#1502 (comment))

glendc added 3 commits November 12, 2022 15:25
expose getEnvParameters from runtime pkg
46c347a
register result of testcase at the end
03fc268 
this way browser can wait for it, given it
lives in a different process than the main driver
improve logger, remove process use and expose env better
1552b79 
- logger needs to handle winston-like usage,
  making sure we log everything nicely as-is,
  instead of passing it directly we first need to manipualte it
  in a winston like manner to get a good effect
- process was used in sync, but we cannot use this,
  env extraction is now cross-inner-package
- this comples the env usage for node-browser-crossing
@GlenDC GlenDC mentioned this pull request Nov 12, 2022
Closed
12 tasks
@GlenDC
Copy link
Contributor Author

GlenDC commented Nov 13, 2022

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

laurentsenta
laurentsenta requested changes Nov 14, 2022
View reviewed changes
Copy link
Contributor

@laurentsenta laurentsenta left a comment •
edited
Loading

Choose a reason for hiding this comment

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

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 EPIC: Clarify and Simplify logging testground#1355 (comment)

@GlenDC
Copy link
Contributor Author

GlenDC commented Nov 15, 2022

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...

GlenDC
GlenDC commented Nov 15, 2022
View reviewed changes
Copy link
Contributor Author

@GlenDC GlenDC left a comment

Choose a reason for hiding this comment

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

LGTM

@laurentsenta laurentsenta changed the title support this sdk in the browser feat: add basic browser support Nov 16, 2022
@laurentsenta
Copy link
Contributor

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

@laurentsenta laurentsenta merged commit 4401d1d into testground:master Nov 16, 2022
@mxinden
Copy link
Member

mxinden commented Nov 16, 2022

Congratulations @GlenDC and @laurentsenta for this milestone!

@BigLep
Copy link

BigLep commented Nov 21, 2022

Agreed - way to go!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@laurentsenta laurentsenta laurentsenta approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@GlenDC @laurentsenta @mxinden @BigLep