Skip to content
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

convert relevant tests to use CLS #31978

Closed
gireeshpunathil opened this issue Feb 27, 2020 · 8 comments
Closed

convert relevant tests to use CLS #31978

gireeshpunathil opened this issue Feb 27, 2020 · 8 comments
Labels
async_hooks Issues and PRs related to the async hooks subsystem.

Comments

@gireeshpunathil
Copy link
Member

Outcome 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

@vdeturckheim
Copy link
Member

@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") :)

@gireeshpunathil
Copy link
Member Author

@vdeturckheim - I haven't look at the tests yet, but will try to look at and find out one, will let you know.

@jasnell
Copy link
Member

jasnell commented 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?

@mhdawson
Copy link
Member

mhdawson commented 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.

@sam-github
Copy link
Contributor

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.

@HarshithaKP
Copy link
Member

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.

HarshithaKP added a commit to HarshithaKP/node that referenced this issue Mar 11, 2020
HarshithaKP added a commit to HarshithaKP/node that referenced this issue Mar 16, 2020
addaleax pushed a commit that referenced this issue Mar 30, 2020
Refs: #31978

PR-URL: #32303
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this issue Mar 30, 2020
Refs: #31978

PR-URL: #32303
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to targos/node that referenced this issue Apr 25, 2020
Refs: nodejs#31978

PR-URL: nodejs#32303
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this issue Apr 28, 2020
Refs: #31978

PR-URL: #32303
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos added the async_hooks Issues and PRs related to the async hooks subsystem. label Dec 27, 2020
@targos
Copy link
Member

targos commented Dec 27, 2020

@gireeshpunathil do you want to keep this issue open?

@gireeshpunathil
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem.
Projects
None yet
Development

No branches or pull requests

7 participants