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

Add ReadableStream support to k6 #3696

Merged
merged 22 commits into from
Apr 29, 2024
Merged

Add ReadableStream support to k6 #3696

merged 22 commits into from
Apr 29, 2024

Conversation

joanlopez
Copy link
Contributor

What?

It adds support for ReadableStream from Streams API through experimental k6/experimental/streams module.

Why?

This is a prerequisite to unlock the development of other features in k6. It is also a nice-to-have feature for k6 users in general, that may want to use ReadableStream on their own.

Note for the reviewer

You can find the rationale behind some technical decisions (around goja, promises, etc) present on this changeset, in this
Google Doc
that I used to write some personal notes, as takeaways during the development process.

On top of that, @oleiade and I decided to keep the code comments for each step of those operations defined in the spec because it makes it easier to:

  • correlate the code and the spec.
  • identify bugs or non-compliant behaviors.
  • identify patterns that are repeated over the spec, and its correspondence in Go/goja.

So, although I know these could be arguable for some people, I'd personally push to keep them as long as I need to maintain and take care of this code.

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make lint) and all checks pass.
  • I have run tests locally (make tests) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

Closes #3659
Contributes to #2978

@joanlopez joanlopez self-assigned this Apr 19, 2024
@joanlopez joanlopez requested a review from a team as a code owner April 19, 2024 13:12
@joanlopez joanlopez requested review from mstoykov, oleiade, codebien and olegbespalov and removed request for a team April 19, 2024 13:12
@joanlopez joanlopez force-pushed the readable-stream branch 2 times, most recently from b434c6e to 3e5ddc9 Compare April 19, 2024 13:53
js/modulestest/runtime.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Apr 19, 2024

Codecov Report

Attention: Patch coverage is 0.28249% with 706 lines in your changes are missing coverage. Please review.

Project coverage is 70.79%. Comparing base (f38ac59) to head (598ac70).

❗ Current head 598ac70 differs from pull request most recent head 89302f2. Consider uploading reports for the commit 89302f2 to get more accurate results

Files Patch % Lines
...odules/k6/experimental/streams/readable_streams.go 0.00% 208 Missing ⚠️
...ntal/streams/readable_stream_default_controller.go 0.00% 134 Missing ⚠️
js/modules/k6/experimental/streams/module.go 0.84% 117 Missing ⚠️
...rimental/streams/readable_stream_default_reader.go 0.00% 63 Missing ⚠️
.../k6/experimental/streams/readable_stream_reader.go 0.00% 59 Missing ⚠️
js/modules/k6/experimental/streams/goja.go 0.00% 44 Missing ⚠️
js/modules/k6/experimental/streams/queue.go 0.00% 25 Missing ⚠️
js/modules/k6/experimental/streams/errors.go 0.00% 24 Missing ⚠️
...dules/k6/experimental/streams/underlying_source.go 0.00% 17 Missing ⚠️
js/modules/k6/experimental/streams/errors_gen.go 0.00% 11 Missing ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3696      +/-   ##
==========================================
- Coverage   73.29%   70.79%   -2.51%     
==========================================
  Files         278      287       +9     
  Lines       20456    21159     +703     
==========================================
- Hits        14994    14979      -15     
- Misses       4505     5217     +712     
- Partials      957      963       +6     
Flag Coverage Δ
ubuntu 70.79% <0.28%> (-2.44%) ⬇️
windows ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mstoykov mstoykov added this to the v0.51.0 milestone Apr 19, 2024
@joanlopez joanlopez force-pushed the readable-stream branch 2 times, most recently from aeb0b07 to c691261 Compare April 24, 2024 10:45
@joanlopez
Copy link
Contributor Author

Hey @mstoykov (cc/ @oleiade),

I have pushed a few more commits with some changes that go in the line of what has been discussed here.

Note that the approach is specific for streams, thus not extensible, but I guess we can postpone the task of finding a way to make the WPTs setup available for other suites as a separate work.

I also took the opportunity to remove the support for console.log, as discussed here, in order to move that discussion outside the scope of this pull request, as it isn't strictly needed.

Please, let know what do you think about the new approach. Thanks! 🙇🏻

Comment on lines 23 to 31
+ // We don't want to rely on the DOM and other browser-based
+ // mechanisms for reporting test failures. Instead, we just
+ // throw the error and make it fail fast, to be aware of it.
+ //
+ // In the future, we might want to add a way to report these
+ // that doesn't rely on a browser, but let all test end before
+ // actually reporting failures.
+ // But, as a first iteration, this approach should suffice.
+ throw `${this.name} failed - ${e}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess here set_status above should be the thing that marks tests as failed?

Copy link
Contributor Author

@joanlopez joanlopez Apr 26, 2024

Choose a reason for hiding this comment

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

Yeah, but that status isn't directly reflected in any way through how we do run the tests, just kept in-memory stored as information associated with the test.

That's why I suggested there to explore a mechanism to do so, like "building" our own test environment, but I'd prefer to leave that for the next iteration as for now it's more than enough (it reports the failures) and the major drawback is that it fail fast and doesn't run all the tests when there's an error, which shouldn't be that bad for an initial release, as long as the behavior is correct.

(cc/ @oleiade as you'll most likely take over 🙇🏻)

mstoykov
mstoykov previously approved these changes Apr 26, 2024
Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

LGTM in general.

I have left some more nits.

I am still not certain about a bunch of the stuff around the running of the tests.

Also if locked is supposed to be read only - the test definitely do not test for this as having as an exported field on a struct let it be set from goja.

So maybe we should have more test than just hte ones in the test suite

oleiade
oleiade previously approved these changes Apr 29, 2024
@oleiade oleiade requested a review from olegbespalov April 29, 2024 07:56
@oleiade oleiade dismissed stale reviews from mstoykov and themself via a35dad0 April 29, 2024 07:57
Copy link
Contributor

@olegbespalov olegbespalov left a comment

Choose a reason for hiding this comment

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

LGTM

@oleiade oleiade requested a review from mstoykov April 29, 2024 08:03
@mstoykov
Copy link
Contributor

Just please squash on merge 😬

@oleiade
Copy link
Member

oleiade commented Apr 29, 2024

👍🏻 😄 🦖

@oleiade oleiade merged commit 3431d7d into master Apr 29, 2024
23 checks passed
@oleiade oleiade deleted the readable-stream branch April 29, 2024 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation-needed A PR which will need a separate PR for documentation feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make ReadableStream (WIP) implementation pass WPT test suite
5 participants