GeezerVison is showing github.com — convert relevant tests to use CLS · Issue #31978 · nodejs/node
convert relevant tests to use CLS #31978DescriptionOutcome of discussion in diagnostics WG
ref: nodejs/diagnostics#361
Is your feature request related to a problem? Please describe.
async_storage API is landed, but may be a long way to go before large scale field adoption - in terms of API stability, interfaces, performance, ...
Describe the solution you'd like
One proven method is to consume it ourselves and allow things to refine and evolve.
We have good amount of client-server tests (net, http, http2...) and a sub-set of those may have opportunities to process data elements through async-storage mechanism. It is so possible that other types of tests have opportunities too.
So the proposal is to:
- develop new tests based on existing qualifying tests (that which pass around contextual data) to use
async_storage
- allow those to run as part of regular CI
- use the outcome as a feeder to the API enhancements
- eventually when things stabilize, remove traditional tests and switch over to the new ones
cc @nodejs/testing @vdeturckheim @Qard Activityvdeturckheim commented on Feb 27, 2020 @gireeshpunathil I really like this plan. But I am unsure of how much work that is atm. Would you have an example of test you would like to see using this API? I guess, based on how many of them there are, we can ask for help around (with "help wanted" and "good first issue") :) Reacted by Gireesh Punathil gireeshpunathil commented on Feb 27, 2020 @vdeturckheim - I haven't look at the tests yet, but will try to look at and find out one, will let you know. Reacted by Vladimir de Turckheim jasnell commented on Feb 27, 2020 While the mechanism is still experimental, I'd be reluctant to switch over to it wholescale so I think the plan to "develop new tests based on existing qualifying tests" is good. I would just be reluctant to require those be green to pass CI. Perhaps moving those into a separate tests/experimental bucket would be good? Reacted by Gireesh Punathil and Vladimir de Turckheim Reacted by Anna Henningsen mhdawson commented on Feb 27, 2020 I'd suggested "develop new tests based on existing qualifying tests" due to the concern (ie changing existing tests to depend on an experimental feature) which I share, but I think that for new tests I think they should be green. If a test can't be green then we should not add it, or potentially mark it as flaky temporarily.
I don't think we've said tests for other experimental features don't have to be green. Reacted by Gireesh Punathil sam-github commented on Mar 2, 2020 I agree, experimental gives us a certain amount of discretion in making breaking changes, or removing the feature, or even having the feature be incomplete in some sense, but we don't knowingly ship bugs in experimental features, or ignore test failures related to them. Reacted by Gireesh Punathil HarshithaKP commented on Mar 4, 2020 I have attempted to port <test/paral/test-http-abort-stream-end.js> to a corresponding cls variant. My plan is to identify unique patterns in the existing bucket, and develop one cls variant test case per pattern.
the general approach is:
- replace deep passing of contextual data with cls store mechanism
- replace closures with non-closure variants and cls stores
please let me know if you have suggestions!
I am also looking to see if there are benchmarks that I can work with to see if non-closure variants can bring-in improvements in terms of performance or footprint.
Reacted by Vladimir de Turckheim and Michael Dawson targos commented on Dec 27, 2020 gireeshpunathil commented on Dec 27, 2020 we made some progress, but does not look like we will go further on this now. Closing this tentatively, can re-open when we decide to pursue this.
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|