Skip to content

Port/Duplicate async_hooks tests to use AsyncLocalStore #55712

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

Open
Flarna opened this issue Nov 4, 2024 · 5 comments
Open

Port/Duplicate async_hooks tests to use AsyncLocalStore #55712

Flarna opened this issue Nov 4, 2024 · 5 comments
Labels
async_local_storage AsyncLocalStorage diag-agenda Issues and PRs to discuss during the meetings of the diagnostics working group.

Comments

@Flarna
Copy link
Member

Flarna commented Nov 4, 2024

Short after #55552 was merged I notices that this resulted in loosing a lot test for AsyncLocalStore.

Before AsyncLocalStore was based on async hooks. There are plenty of tests for async hooks which implict verified that AsyncLocalStore does what it is expected.

But since #55552 AsyncLocalStore and async hooks are independent.

I don't think we have to dup them all, but there are some special cases at least in HTTP area (e.g. here) which might require followups.

fyi @nodejs/diagnostics

Edit 21.11.: corrected link to PR

@Flarna Flarna added the async_local_storage AsyncLocalStorage label Nov 4, 2024
@Flarna Flarna changed the title Port/Duplicte async_hooks tests to AsyncLocalStore Port/Duplicate async_hooks tests to use AsyncLocalStore Nov 6, 2024
@Qard Qard added the diag-agenda Issues and PRs to discuss during the meetings of the diagnostics working group. label Nov 21, 2024
@Qard
Copy link
Member

Qard commented Nov 21, 2024

#54879 got merged? It looks like it was just closed to wait for v24.

In any case, I've added the drag-agenda label. Let's discuss this in more detail in the next diagnostics working group call. (Not the one today...probably a bit short notice for that.)

@Flarna
Copy link
Member Author

Flarna commented Nov 21, 2024

No it wasn't. Will reopen for 24 at some time.

Anyhow, just noticed that I linked the wrong PR. It is about yours which was merged a while ago: #55552 That one changes internal how ALS works and relying on AsyncHooks tests is no longer applicable.

@Qard
Copy link
Member

Qard commented Nov 25, 2024

It swaps the default for the flag, but all tests with the test-async-local-storage- prefix are run both with and without he changes, so test coverage should remain the same.

@Flarna
Copy link
Member Author

Flarna commented Nov 26, 2024

Yes, they run with both. I'm talking more about the tests which don't have the test-async-local-storage- prefix.
e.g. test-crypto-randomBytes verifies init/before/after/destroy for async-hooks and as long as this test works an async hooks based ALS will likely work fine.
or test-async-exec-resource-http-32060 which has also no variant using test-async-local-storage- prefix therefore ALS coverage for that case is lost.

@Qard
Copy link
Member

Qard commented Nov 29, 2024

Ah, well if there's things which are testing ALS-related behaviour that aren't actually marked as such then there should probably be some tests specific to ALS for those scenarios.

nodejs-github-bot pushed a commit that referenced this issue Apr 27, 2025
Add a test to verify AsyncLocalStore functionality for HTTP using a
keep alive agent.

AsyncLocalStore moves away from using async_hooks therefore relying on
async_hooks tests alone is not longer valid.

PR-URL: #58017
Refs: #55712
Refs: #13325
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: James M Snell <[email protected]>
RafaelGSS pushed a commit that referenced this issue May 1, 2025
Add a test to verify AsyncLocalStore functionality for HTTP using a
keep alive agent.

AsyncLocalStore moves away from using async_hooks therefore relying on
async_hooks tests alone is not longer valid.

PR-URL: #58017
Refs: #55712
Refs: #13325
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: James M Snell <[email protected]>
RafaelGSS pushed a commit that referenced this issue May 2, 2025
Add a test to verify AsyncLocalStore functionality for HTTP using a
keep alive agent.

AsyncLocalStore moves away from using async_hooks therefore relying on
async_hooks tests alone is not longer valid.

PR-URL: #58017
Refs: #55712
Refs: #13325
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_local_storage AsyncLocalStorage diag-agenda Issues and PRs to discuss during the meetings of the diagnostics working group.
Projects
None yet
Development

No branches or pull requests

2 participants