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.

Sign up for GitHub

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

Jump to bottom

test: rewrite fs {f}utimes test file #25656

Closed
wants to merge 1 commit into from
Closed

test: rewrite fs {f}utimes test file #25656

wants to merge 1 commit into from

Conversation

Fishrock123
Copy link
Contributor

@Fishrock123 Fishrock123 commented Jan 23, 2019 •
edited
Loading

Previously this test silently swallowed some errors.
Hint: That giant callback stack.

Refactored to use common.mustCall() & assert()s.

Also, this adds a couple of extra error-checking cases.

Ugh I'm sorry about this one. I've been meaning to do it forever. (I tired to get someone else to do it as their first PR and that never panned out.)

Anyways this was originally discovered as part of lionheart/openradar-mirror#19944

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

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

@Fishrock123 Fishrock123 added fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests. labels Jan 23, 2019
@nodejs-github-bot
Copy link
Collaborator

@Fishrock123 build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2352/pipeline

@Fishrock123
Copy link
Contributor Author

Fishrock123 commented Jan 23, 2019 •
edited
Loading

Weird. I can't reset the Ci results without doing a full run but they are actually all green. Linuxone failed from Travis (EADDRINUSE..), the actual one is at https://ci.nodejs.org/job/node-test-commit-linuxone/9582/

EDIT: ok just gona rebuild and hopefully get that ✅... https://ci.nodejs.org/job/node-test-pull-request/20288/

Edit, again, I guess, why not: https://ci.nodejs.org/job/node-test-pull-request/20308/

Sigh, ok rebased to hopefully work after that test fix: https://ci.nodejs.org/job/node-test-pull-request/20377/

@Fishrock123 Fishrock123 mentioned this pull request Jan 23, 2019
Closed
@Fishrock123
test: rewrite fs {f}utimes test file
7939e48 
Previously this test silently swallowed some errors.

Refactored to use `common.mustCall()` & `assert()`s.

Also, this adds a couple of extra error-checking cases.
@Fishrock123
Copy link
Contributor Author

@nodejs/testing maybe?

@Trott
Copy link
Member

Trott commented Jan 28, 2019

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

@Trott
Copy link
Member

Trott commented Jan 28, 2019

Maybe @nodejs/fs as well?

@addaleax
Copy link
Member

Landed in 148cac2

@addaleax addaleax closed this Jan 28, 2019
addaleax pushed a commit that referenced this pull request Jan 28, 2019
@Fishrock123 @addaleax
test: rewrite fs {f}utimes test file
148cac2 
Previously this test silently swallowed some errors.

Refactored to use `common.mustCall()` & `assert()`s.

Also, this adds a couple of extra error-checking cases.

PR-URL: #25656
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax pushed a commit that referenced this pull request Jan 28, 2019
@Fishrock123 @addaleax
test: rewrite fs {f}utimes test file
77274d0 
Previously this test silently swallowed some errors.

Refactored to use `common.mustCall()` & `assert()`s.

Also, this adds a couple of extra error-checking cases.

PR-URL: #25656
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@Fishrock123 Fishrock123 deleted the fix-fs-utimes-test branch January 28, 2019 23:47
@targos targos mentioned this pull request Jan 29, 2019
Merged
BethGriggs pushed a commit that referenced this pull request Apr 29, 2019
@Fishrock123 @BethGriggs
test: rewrite fs {f}utimes test file
0f34794 
Previously this test silently swallowed some errors.

Refactored to use `common.mustCall()` & `assert()`s.

Also, this adds a couple of extra error-checking cases.

PR-URL: #25656
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@BethGriggs BethGriggs mentioned this pull request May 1, 2019
Merged
BethGriggs pushed a commit that referenced this pull request May 10, 2019
@Fishrock123 @BethGriggs
test: rewrite fs {f}utimes test file
b5a641f 
Previously this test silently swallowed some errors.

Refactored to use `common.mustCall()` & `assert()`s.

Also, this adds a couple of extra error-checking cases.

PR-URL: #25656
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
@Fishrock123 @MylesBorins
test: rewrite fs {f}utimes test file
1afd614 
Previously this test silently swallowed some errors.

Refactored to use `common.mustCall()` & `assert()`s.

Also, this adds a couple of extra error-checking cases.

PR-URL: #25656
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
This was referenced May 29, 2019
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
Labels
fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Fishrock123 @nodejs-github-bot @Trott @addaleax @BridgeAR