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

test: improve clarity of test-async-local-storage-enable-disable.js \… #38008

Closed
wants to merge 1 commit into from

Conversation

PhakornKiong
Copy link
Contributor

The last als.run() will reactivate the als, hence the test should test for getting the object, not undefined

Based on the Nodejs documentation

Disables the instance of AsyncLocalStorage. All subsequent calls to asyncLocalStorage.getStore() will return undefined until asyncLocalStorage.run() or asyncLocalStorage.enterWith() is called

The original test is doing

asyncLocalStorage.run(new Map(), () => {
        assert.notStrictEqual(asyncLocalStorage.getStore(), undefined);

The asyncLocalStorage.getStore() will return a Map, however, the test will pass as it is not strict.

As this is within a run(), we should test for asyncLocalStorage to return the store instead.

This will demonstrate a clearer purpose of the test.

@nodejs-github-bot nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Mar 31, 2021
@Flarna
Copy link
Member

Flarna commented Apr 1, 2021

Please adapt the commit message to follow the guideline - it's too long for a headline.

The last als.run() will reactivate the als,
hence the test should test for getting the object,
not undefined
@PhakornKiong
Copy link
Contributor Author

@Flarna I've updated the commit message.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/37082/

@Flarna Flarna added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 1, 2021
Flarna pushed a commit that referenced this pull request Apr 2, 2021
The last als.run() will reactivate the als,
hence the test should test for getting the object,
not undefined

PR-URL: #38008
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@Flarna
Copy link
Member

Flarna commented Apr 2, 2021

Landed in e46c680

@Flarna Flarna closed this Apr 2, 2021
MylesBorins pushed a commit that referenced this pull request Apr 4, 2021
The last als.run() will reactivate the als,
hence the test should test for getting the object,
not undefined

PR-URL: #38008
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Apr 4, 2021
targos pushed a commit that referenced this pull request May 1, 2021
The last als.run() will reactivate the als,
hence the test should test for getting the object,
not undefined

PR-URL: #38008
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@danielleadams danielleadams mentioned this pull request May 3, 2021
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. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants