Web Platform Tests Checkout #87
Conversation
olegbespalov
requested review from
mstoykov and
oleiade
and removed request for
a team
olegbespalov
force-pushed
the
feat/web-platform-tests
branch
from
e6e8359 to
9637d44
Compare
|
I'll name @joanlopez for this review in my place if that's okay with you folks, as he drove this work on streams before and would likely have much more insights to offer than me 🙇🏻 |
|
|
||
| gotErr := ts.EventLoop.Start(func() error { | ||
| err := executeTestScripts(ts.VU.Runtime(), "./tests/generateKey", "successes.js") | ||
| err := executeTestScripts(ts.VU.Runtime(), webPlatformTestSuite+"generateKey", "successes.js") |
There was a problem hiding this comment.
I pointed out this in the RSA PR, but a lot of those test seem very similar and likely can be a table test instead.
| - var subtle = self.crypto.subtle; | ||
| + var subtle = crypto.subtle; |
There was a problem hiding this comment.
Again I comment on this in the RSA test , but we can set globalThis.self to globalThis and won't need to do this everywhere.
| - done(); | ||
| - | ||
| - |
There was a problem hiding this comment.
This seems very strange ... Can we maybe have comments in the places where we do strange changes :)
There was a problem hiding this comment.
Thank you for doing this nad LGTM in general.
I would really prefer if every change has a comment on why we are doing this. Some do whcih is great, but some that seem "strange" do not and I wonder to what extend that is "this is not needed for us" and "we have not implemented something" or "we implemented something wrongly".
I would argue (as I have done for other test suties including tc39's test262) to just add empty implementations in cases where we do not have to do something special for example, instead of changingthe tests
I do accept all the comments and will be happy to implement them, but probably as part of the following PR since I'd like to merge firstly this and RSA support. I will open take anyway 👍 |
|
@joanlopez I've merged to run the PRs train. Feel free to drop more feedback here since in any case, soon I'll open one more to address Mihails suggestions |
There was a problem hiding this comment.
@joanlopez I've merged to run the PRs train. Feel free to drop more feedback here since in any case, soon I'll open one more to address Mihails suggestions
Hey @olegbespalov! I did review these changes and I don't have any comment that isn't covered yet by Mihail's suggestions, so 👌🏻 Great work! 🚀
What?
This PR implements running of the web platform tests not against static in-repository files, but it does check out to the latest proven to commit, apply patches with k6/web crypto implementation specific, and runs test against it.
It also brings tooling around, like easy checkout and patch generation.
Why?
This approach has proven successful in grafana/k6#3696 and has many benefits. For example, we can clearly identify the difference between K6's and WPT test suites.
It also improves developer (and reviewer) UX.
Closes: #81