-
Notifications
You must be signed in to change notification settings - Fork 509
Port some internal streams tests to wd-test format #5725
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
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
|
Did you validate that these are 1 to 1 replica of the ew-tests? It's hard to review. Knowing that you've validated will ease the review process. |
|
@anonrig ...Please go ahead with whatever additional changes you feel are necessary here. Feel free to push additional commits. Before you do so, it might be easier to merge the other PR into this one to save some rebasing trouble later. |
f9aba30 to
33674b0
Compare
33674b0 to
dec9219
Compare
3902808 to
41647a0
Compare
jasnell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This likely needs a bit more of a looking at to make sure all of the ported tests are actually from ew-tests and not just incorrectly copied from already existing workerd tests.
1cb76ab to
9c630fe
Compare
harrishancock
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've gotten partway through pipe-streams-test, but need to give my brain a rest.
Most of my comments so far are non-blocking observations or nits, I think, but there are a few real issues.
|
For those reviewing.. just as an FYI... @anonrig has taken this PR over. |
|
Fixing conflicts before addressing the comments. |
d59f41b to
bbcd7e0
Compare
bbcd7e0 to
b759d3a
Compare
b759d3a to
b2a9298
Compare
Co-authored-by: James M Snell <[email protected]>
|
@guybedford @harrishancock would you mind reviewing this? i'd like to land this asap so that we can iteratively improve the test coverage. |
Co-authored-by: Yagiz Nizipli <[email protected]>
Had claude port a handful of the easier-to-port tests and provide a checklist for doing the rest. Some will require manual figuring out to port, most likely can be automated with claude.
PR is tests and doc only. No functional changes. As long as CI passes they should be good. Once existing tests have been ported, a separate analysis of test coverage will be performed to determine what is missing.
Current state visible to Cloudflare employees: https://docs.google.com/document/d/178N1WCas0PLO4nLnr597daR5XI8NRo3lRQW3G7mOvHI/edit?tab=t.0