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.

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

Implement Browser.on(event) #268

Merged
merged 6 commits into from
Mar 14, 2022
Merged

Implement Browser.on(event) #268

merged 6 commits into from
Mar 14, 2022

Conversation

imiric
Copy link
Contributor

@imiric imiric commented Mar 8, 2022

This is an implementation of Browser.on(event), using the recently released event loop support in k6. The only supported event is disconnected, so it can be used as an alternative to teardown().

It also does a few minor fixes and refactors. See the commits for details.

Closes #95

@imiric imiric requested a review from inancgumus March 8, 2022 17:05
common/browser.go Outdated Show resolved Hide resolved
common/browser.go Outdated Show resolved Hide resolved
common/browser.go Show resolved Hide resolved
examples/browser_on.js Show resolved Hide resolved
Copy link
Member

@inancgumus inancgumus left a comment

Choose a reason for hiding this comment

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

Here are my suggestions.

common/types.go Outdated Show resolved Hide resolved
common/context.go Outdated Show resolved Hide resolved
examples/browser_on.js Show resolved Hide resolved
examples/browser_on.js Outdated Show resolved Hide resolved
common/browser.go Outdated Show resolved Hide resolved
common/browser.go Show resolved Hide resolved
@inancgumus
Copy link
Member

inancgumus commented Mar 9, 2022

@imiric, by the way, we don't return here even though the connection is closed:

func (b *Browser) initEvents() error {
	...
	go func() {
		for {
			select {
			case <-cancelCtx.Done():
				return
			case event := <-chHandler:
				...
				} else if event.typ == EventConnectionClose {
					...
					b.browserProc.didLoseConnection()
					b.cancelFn()

					// LOOK HERE: it makes sense to return here
				}
			}
		}
	}()

This might be a bug (even though the next iteration will catch that the context is being closed). I wanted to let you know.

imiric pushed a commit that referenced this pull request Mar 9, 2022
imiric pushed a commit that referenced this pull request Mar 9, 2022
imiric pushed a commit that referenced this pull request Mar 9, 2022
As discussed here[1][2], sleep() was only used in order to test the
Promise rejection scenario. Since pressing ^C currently doesn't trigger
it, and we should have a Go integration test that simply cancels the
context, this sleep() is not needed anymore.

[1]: #268 (comment)

[2]: #268 (comment)
@imiric imiric force-pushed the feat/95-browser-on branch 3 times, most recently from 118e9df to 7f51f8a Compare March 9, 2022 17:38
imiric pushed a commit that referenced this pull request Mar 10, 2022
imiric pushed a commit that referenced this pull request Mar 10, 2022
As discussed here[1][2], sleep() was only used in order to test the
Promise rejection scenario. Since pressing ^C currently doesn't trigger
it, and we should have a Go integration test that simply cancels the
context, this sleep() is not needed anymore.

[1]: #268 (comment)

[2]: #268 (comment)
@imiric imiric force-pushed the feat/95-browser-on branch 3 times, most recently from 2c62667 to a0e8d14 Compare March 10, 2022 16:12
imiric pushed a commit that referenced this pull request Mar 10, 2022
@imiric imiric force-pushed the feat/95-browser-on branch from c3b0dce to 0eb8df4 Compare March 11, 2022 15:22
imiric pushed a commit that referenced this pull request Mar 11, 2022
@imiric imiric force-pushed the feat/95-browser-on branch from 0eb8df4 to 6f0dd9a Compare March 11, 2022 15:41
imiric pushed a commit that referenced this pull request Mar 11, 2022
@imiric imiric force-pushed the feat/95-browser-on branch from 6f0dd9a to 51de7f9 Compare March 11, 2022 15:42
@imiric imiric force-pushed the feat/95-browser-on branch from 13986c3 to 5ccc80e Compare March 11, 2022 16:27
@imiric imiric marked this pull request as ready for review March 11, 2022 16:38
@imiric imiric requested a review from inancgumus March 11, 2022 16:38
@imiric imiric changed the title WIP Implement Browser.on(event) Implement Browser.on(event) Mar 11, 2022
Ivan Mirić added 5 commits March 11, 2022 17:45
This ensures the WS connection is cleanly closed, so that we wait for
and process EventConnectionClose. It should also minimize
"close 1006 (abnormal closure): unexpected EOF" errors[1] on shutdown.

[1]: https://community.k6.io/t/erro-0006-unexpected-eof-error-during-run-of-example-script/3136
The k6 context-based utils were deprecated[1], and will be removed in
v0.38.0[2], so this attaches VU to the context so that its helper
functions can be used instead. Right now it will only be used for
RegisterCallback(), but we should move all other uses of it soon (mainly
GetRuntime()).

[1]: grafana/k6@335d99c

[2]: grafana/k6#2385
@imiric imiric force-pushed the feat/95-browser-on branch from 5ccc80e to 82a601f Compare March 11, 2022 16:47
Copy link
Member

@inancgumus inancgumus left a comment

Choose a reason for hiding this comment

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

Nice job.

@imiric imiric added this to the v0.3.0 milestone Mar 14, 2022
@imiric imiric merged commit 24bf7a3 into main Mar 14, 2022
@imiric imiric deleted the feat/95-browser-on branch March 14, 2022 11:42
@inancgumus inancgumus removed this from the v0.3.0 milestone Mar 14, 2022
imiric pushed a commit that referenced this pull request Mar 29, 2022
This drops all usage of context-based getter functions (removed from k6 in [1])
in favor of methods attached to the VU instance passed to the extension.
We attached this VU instance to the internal context in #268 to keep a
similar API, but in the future this should be refactored to avoid the
use of context.

[1]: grafana/k6@64f17c7
imiric pushed a commit that referenced this pull request Mar 30, 2022
This drops all usage of context-based getter functions (removed from k6 in [1])
in favor of methods attached to the VU instance passed to the extension.
We attached this VU instance to the internal context in #268 to keep a
similar API, but in the future this should be refactored to avoid the
use of context.

[1]: grafana/k6@64f17c7
@imiric imiric mentioned this pull request Apr 19, 2022
imiric referenced this pull request in grafana/k6-docs Jul 11, 2022
- Add the tooltip to the emoji to descrive what it means.
- If there is an issue open for the feature then link to it from the
  emoji.
Resolves: #722 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Browser.on(event)
3 participants