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: fix unreliable test-fs-stat-bigint #21949

Closed
wants to merge 2 commits into from

Conversation

samarthgulati
Copy link

Remove side-effects between testcases by creating a new file everytime.

Fixes: #21948

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Remove side-effects between testcases by creating a new file everytime.

Fixes: nodejs#21948
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jul 23, 2018
@Trott
Copy link
Member

Trott commented Jul 23, 2018

@Trott Trott added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. flaky-test Issues and PRs related to the tests with unstable failures on the CI. labels Jul 23, 2018
}

function linkToFile(filename) {
const link = path.join(tmpdir.path, `symbolic-link-${testIndex}`);
Copy link
Member

Choose a reason for hiding this comment

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

Should this maybe also increment the index?

Copy link
Member

@Trott Trott Jul 23, 2018

Choose a reason for hiding this comment

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

That wouldn't hurt but also isn't necessary?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe another fun option might be to change it from:

`symbolic-link-${testIndex}`

...to:

`${filename}-link`

Copy link
Member

Choose a reason for hiding this comment

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

...which might make the whole function unnecessary? All calls to the function could be replaced with:

fs.symlinkSync(filename, `${filename}-link`);

Copy link
Author

Choose a reason for hiding this comment

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

Incorporated the changes you suggested. I still have the link variable in there to keep the line width under limit.

@Trott
Copy link
Member

Trott commented Jul 24, 2018

@trivikr
Copy link
Member

trivikr commented Jul 24, 2018

@samarthgulati Congratulations on your first PR to Node.js core! 🎉
Looks like user.email in git config is not the same as your email registered with Github. Either add currently used email ID to Github, or update your email ID as per the instructions here

Trott pushed a commit to Trott/io.js that referenced this pull request Jul 25, 2018
Remove side-effects between testcases by creating a new file everytime.

Fixes: nodejs#21948
PR-URL: nodejs#21949
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@Trott
Copy link
Member

Trott commented Jul 25, 2018

Landed in 4d94bb2.

Thanks for the contribution! 🎉

(If you're interested in other possible contributions to Node.js but don't have a good idea of where to start looking, come ideas are posted at https://www.nodetodo.org/next-steps/.)

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. flaky-test Issues and PRs related to the tests with unstable failures on the CI. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fix flaky test-fs-stat-bigint
6 participants