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: add missing await in fs-rm/fs-rmdir tests #41545

Closed
wants to merge 1 commit into from

Conversation

bcoe
Copy link
Contributor

@bcoe bcoe commented Jan 15, 2022

Noticed that a few assertions were not being awaited, this could
potentially be leading to flakiness in tmp cleanup.

Refs: #41201

@bcoe bcoe requested review from iansu and mhdawson January 15, 2022 18:31
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Jan 15, 2022
Copy link
Contributor

@iansu iansu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into this Ben.

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jan 15, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 15, 2022
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit on the commit message: The first word after the subsystem is supposed to be an imperative mood verb. So maybe this?

test: add missing await in fs-rm/fs-rmdir tests

@bcoe bcoe changed the title test: fs-rm fs-rmdir-recursive, ensure awaited test: add missing await in fs-rm/fs-rmdir tests Jan 16, 2022
Noticed that a few assertions were not being awaited, this could
potentially be leading to flakiness in tmp cleanup.

Refs: nodejs#41201
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

mhdawson pushed a commit that referenced this pull request Jan 17, 2022
Noticed that a few assertions were not being awaited, this could
potentially be leading to flakiness in tmp cleanup.

Refs: #41201

PR-URL: #41545
Refs: #41201
Reviewed-By: Ian Sutherland <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@mhdawson
Copy link
Member

Landed in 6b9d2ae

@mhdawson mhdawson closed this Jan 17, 2022
thedull pushed a commit to thedull/node that referenced this pull request Jan 18, 2022
Noticed that a few assertions were not being awaited, this could
potentially be leading to flakiness in tmp cleanup.

Refs: nodejs#41201

PR-URL: nodejs#41545
Refs: nodejs#41201
Reviewed-By: Ian Sutherland <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
BethGriggs pushed a commit that referenced this pull request Jan 25, 2022
Noticed that a few assertions were not being awaited, this could
potentially be leading to flakiness in tmp cleanup.

Refs: #41201

PR-URL: #41545
Refs: #41201
Reviewed-By: Ian Sutherland <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
Noticed that a few assertions were not being awaited, this could
potentially be leading to flakiness in tmp cleanup.

Refs: nodejs#41201

PR-URL: nodejs#41545
Refs: nodejs#41201
Reviewed-By: Ian Sutherland <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
danielleadams pushed a commit that referenced this pull request Feb 26, 2022
Noticed that a few assertions were not being awaited, this could
potentially be leading to flakiness in tmp cleanup.

Refs: #41201

PR-URL: #41545
Refs: #41201
Reviewed-By: Ian Sutherland <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.