-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
test: cls variant of test-http-abort-stream-end #32084
base: main
Are you sure you want to change the base?
Conversation
We should probably agree on whether we should use als or cls for the naming. |
@mhdawson, cls and als are used interchangeably in test cases. Confused which one to use. Open for suggestions. |
Are there other test cases with cls in the name? If not I'd probably go with als |
@mhdawson, thanks. Other test cases are named with als. I will change the name to als. |
@mhdawson, Original test cases coming along with API are named |
Refs: nodejs#31978 fixup: address review comment
fixup: address review comment
1d83059
to
ce72d04
Compare
I think I'd still use |
@mhdawson, Changed the name to als, as there was no previous use of cls. |
@HarshithaKP Did you mean to put the test file into another directly, e.g. parallel? |
@addaleax - I meant experimental folder itself, based on the suggestion at #31978 (comment) |
Hm, okay … I don’t quite see the point of having tests that are allowed to fail, but in that case I would
But again, if something’s allowed to fail, then imo let’s just put it in parallel/ and mark it as flaky if an issue with it comes up. |
I also do not think putting them into experimental makes sense. If they are allowed to fail I don't see the value unless it is temporary and in that case we should just wait until they do pass before adding them. |
Sorry last comment should have said do NOT think. Updated to reflect that |
Moved test case to parallel folder. |
@HarshithaKP, @gireeshpunathil I went to look at the original test. I had thought the idea was to find tests that had needed/used something similar to als as part of the http tests and then make versions which used als as an additional more real-world test of als. I'm not really sure this matches that case as it seems to use globals which work just fine in this case. Can you add some details as why this was one of the tests that was a good fit? |
@mhdawson, This test does not use any new globals. The original test was making use of a data handler as a closure, which accumulated incoming data into a buffer, aborted the request half way, and made assertions on the aborted state and the data size. we converted data handler to a non-closure function and used So I guess this is a good fit in terms of usage of als to store contextual data (incoming data size in this case) rather than relying on passing around, or holding up in a closure. |
@HarshithaKP I'm not sure replacing the use of closures it was I expected/thought made sense. Maybe we can discuss in more in depth in the next Diagnostics WG meeting. |
8ae28ff
to
2935f72
Compare
@HarshithaKP do you still want to merge this? If so, can you please rebase on top of |
Refs: #31978
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes