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: change test limit for atime from 2ms to 5ms #30437

Closed
wants to merge 2 commits into from

Conversation

duncanhealy
Copy link
Contributor

doc: change test limit for atime from 2ms to 5ms
fixes #24593

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 12, 2019
@Trott
Copy link
Member

Trott commented Nov 12, 2019

I'm concerned that this change masks an actual bug. What's the reason to believe it doesn't?

@addaleax
Copy link
Member

@Trott I think there are two changes which got mixed together here…

As for increasing the timeout, I’m okay because the test is just inherently flaky the way it’s written and we can’t really do anything about that.

@Trott
Copy link
Member

Trott commented Nov 12, 2019

As for increasing the timeout, I’m okay because the test is just inherently flaky the way it’s written and we can’t really do anything about that.

@addaleax Flaky due to rounding errors, or something else?

@addaleax
Copy link
Member

@Trott It’s flaky because the stat calls for the regular version and the bigint version are made at different times, and so they leave a window for stat data changing in between, or at least that’s my understanding of where this comes from

@duncanhealy
Copy link
Contributor Author

As for increasing the timeout, I’m okay because the test is just inherently flaky the way it’s written and we can’t really do anything about that.

@addaleax Flaky due to rounding errors, or something else?

lstat running twice first with {bigint:true} passed as parameter, on this test I was constistently getting 4ms difference for atime

@Trott It’s flaky because the stat calls for the regular version and the bigint version are made at different times, and so they leave a window for stat data changing in between, or at least that’s my understanding of where this comes from

Could we change the nested call instead? L135-139 test-fs-stat-bigint.js

 fs.lstat(link, { bigint: true }, (err, bigintStats) => {
    fs.lstat(link, (err, numStats) => {
      verifyStats(bigintStats, numStats);
    });

@Trott
Copy link
Member

Trott commented Nov 12, 2019

@Trott It’s flaky because the stat calls for the regular version and the bigint version are made at different times, and so they leave a window for stat data changing in between, or at least that’s my understanding of where this comes from

Sounds good to me. Thanks! Maybe a comment explaining that would be useful.

/ping @nodejs/testing

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 30, 2019
@nodejs-github-bot
Copy link
Collaborator

@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 30, 2019
@addaleax
Copy link
Member

@duncanhealy Could you rebase this against the master branch, or squash the commits here together? Our CI doesn’t deal well with merge conflicts.

@Trott
Copy link
Member

Trott commented Nov 30, 2019

Subsystem should be test rather than doc. If this gets fixed in a rebase or whatever, then great. But if not, then it can be fixed when landing.

@duncanhealy duncanhealy changed the title doc: change test limit for atime from 2ms to 5ms test: change test limit for atime from 2ms to 5ms Nov 30, 2019
@duncanhealy
Copy link
Contributor Author

@duncanhealy Could you rebase this against the master branch, or squash the commits here together? Our CI doesn’t deal well with merge conflicts.

changes squashed

test/parallel/test-fs-stat-bigint.js Outdated Show resolved Hide resolved
@Trott
Copy link
Member

Trott commented Feb 7, 2020

The test does not seem to be failing anymore, so I'm going to close this. Feel free to re-open if you think that's the wrong thing to do and this should land regardless. I'm not acting on a strong opinion. I'm just doing a little tidying up. Thanks!

@Trott Trott closed this Feb 7, 2020
@Trott
Copy link
Member

Trott commented Feb 8, 2020

Stress test on FreeBSD shows this is still A Thing. Re-opening.

@Trott Trott reopened this Feb 8, 2020
@lundibundi
Copy link
Member

@Trott this actually fails for me locally on Arch Linux every now and then (the 2ms), so I'd be in favor of merging this unless we find a better solution.

@nodejs-github-bot

This comment has been minimized.

@Trott
Copy link
Member

Trott commented Feb 9, 2020

I think if we update the comment and move the 5 magic number to a variable that has the comment right above it, this is good to land (with a clean CI).

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.

LGTM with nits

@Trott Trott force-pushed the test-fs-stat-bigint branch 2 times, most recently from c1f6a81 to ac23f40 Compare February 9, 2020 11:23
@Trott
Copy link
Member

Trott commented Feb 9, 2020

I addressed the nits, updated the commit message, and pushed the changes to the PR branch.

@nodejs-github-bot

This comment has been minimized.

@Trott
Copy link
Member

Trott commented Feb 9, 2020

@lundibundi Do the changes I made just now look good to you? If so, feel free to add the author ready label (or land it if CI is finished and green/yellow).

@lundibundi lundibundi added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 9, 2020
duncanhealy and others added 2 commits February 9, 2020 01:32
Change test limit for atime from 2ms to 5ms. Add comment explaining why
the wiggle room is needed.

Fixes: nodejs#24593
@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Feb 11, 2020

So, this doesn't fully fix the test, but it fixes the issue with times being off. There's still an issue with the block count being wrong, somehow, but I'll open a separate issue for that after this lands.

@Trott
Copy link
Member

Trott commented Feb 11, 2020

Landed in 5b0308c...fb437c4

@Trott Trott closed this Feb 11, 2020
Trott pushed a commit to Trott/io.js that referenced this pull request Feb 11, 2020
Change test limit for atime from 2ms to 5ms. Add comment explaining why
the wiggle room is needed.

Fixes: nodejs#24593

PR-URL: nodejs#30437
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Trott added a commit to Trott/io.js that referenced this pull request Feb 11, 2020
PR-URL: nodejs#30437
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this pull request Feb 17, 2020
Change test limit for atime from 2ms to 5ms. Add comment explaining why
the wiggle room is needed.

Fixes: #24593

PR-URL: #30437
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this pull request Feb 17, 2020
PR-URL: #30437
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebytere codebytere mentioned this pull request Feb 17, 2020
codebytere pushed a commit that referenced this pull request Mar 15, 2020
Change test limit for atime from 2ms to 5ms. Add comment explaining why
the wiggle room is needed.

Fixes: #24593

PR-URL: #30437
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this pull request Mar 15, 2020
PR-URL: #30437
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this pull request Mar 17, 2020
Change test limit for atime from 2ms to 5ms. Add comment explaining why
the wiggle room is needed.

Fixes: #24593

PR-URL: #30437
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this pull request Mar 17, 2020
PR-URL: #30437
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebytere codebytere mentioned this pull request Mar 17, 2020
codebytere pushed a commit that referenced this pull request Mar 30, 2020
Change test limit for atime from 2ms to 5ms. Add comment explaining why
the wiggle room is needed.

Fixes: #24593

PR-URL: #30437
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this pull request Mar 30, 2020
PR-URL: #30437
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
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. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test /parallel/test-fs-stat-bigint fails 3 out of 100 times
6 participants