Port some internal streams tests to wd-test format #5725
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. |
anonrig
force-pushed
the
jasnell/port-some-internal-streams-tests
branch
from
f9aba30 to
33674b0
Compare
anonrig
force-pushed
the
jasnell/port-some-internal-streams-tests
branch
from
33674b0 to
dec9219
Compare
anonrig
force-pushed
the
jasnell/port-some-internal-streams-tests
branch
2 times, most recently
from
7729442 to
3902808
Compare
anonrig
force-pushed
the
jasnell/port-some-internal-streams-tests
branch
from
3902808 to
41647a0
Compare
| let cancelCalled = false; | ||
| const { readable } = new TransformStream({ | ||
| async cancel(reason) { | ||
| strictEqual(reason, 'boom'); | ||
| await scheduler.wait(10); | ||
| cancelCalled = true; | ||
| }, |
There was a problem hiding this comment.
Do these negative diffs retain coverage I don't see the cancelCalled variable retained in the wd-test?
There was a problem hiding this comment.
we now use mock.fn() from node:test
There was a problem hiding this comment.
While this is a net improvement to the test, I would have liked to see these types of changes deferred to either a separate commit or a separate PR as they make reconciling the ported tests to the original tests more difficult. Specifically, as @guybedford points out, it makes it more difficult to know that the coverage has remained the same when comparing the original and new side-by-side. Consider it non-blocking tho.
There was a problem hiding this comment.
We couldn't do it because we had to force push to get rid of information leak through the original commit.
There was a problem hiding this comment.
That doesn't really make sense. the todo doc could have been removed from the original commit without flattening all of the changes to the non-related files. Also, these changes could have been deferred until the main set was landed. Either way, it's done now. Let's make sure it's a bit clearer moving forward as we do need to be able to reconcile with the original tests.
| }); | ||
| strictEqual(cancelFn.mock.callCount(), 0); | ||
| await readable.cancel('boom'); | ||
| strictEqual(cancelFn.mock.callCount(), 1); |
There was a problem hiding this comment.
callCount is bumped before scheduler.wait though right? So in theory the test changed?
jasnell
left a comment
jasnell
left a comment
There was a problem hiding this comment.
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.
| // - sync-error-during-flush: Ported as syncErrorDuringFlush | ||
| // - async-error-during-flush: Ported as asyncErrorDuringFlush | ||
| // - write-backpressure: Ported as writeBackpressure | ||
| // - transform-stream-cancel: Ported as transformStreamCancel |
There was a problem hiding this comment.
I don't know exactly how claude came up with some of these but it needs to be checked. transform-stream-cancel does not exist in the internal tests. Instead it looks like transformStreamCancel was adopted from workerd/api/streams-test.js! It's entirely possible that claude was confused. When I put together the initial draft of this I had not yet gone through and fully audited these.
4 participants
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.