Skip to content

Conversation

PatrickHeneise
Copy link

Refactored fs.write() tests to use done, thanks for your help @mcollina.

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 6, 2017
@mscdex mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Nov 6, 2017
}));
});

fs.write(fd, '', 0, 'utf8', function(err, written) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There probably needs to be a common.mustCall() for this callback as well.

Copy link
Author

@PatrickHeneise PatrickHeneise Nov 6, 2017

Choose a reason for hiding this comment

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

Like this:

  const written = common.mustCall(function(err, written) {
    assert.strictEqual(0, written);
  });

  fs.write(fd, '', 0, 'utf8', written);

?

Copy link
Contributor

Choose a reason for hiding this comment

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

Either inline or not, doesn't really matter.

assert.strictEqual(expected, found);
});

fs.write(fd, '', 0, 'utf8', (err, written) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@mcollina mcollina added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Nov 7, 2017
@mcollina
Copy link
Member

mcollina commented Nov 7, 2017

@Trott
Copy link
Member

Trott commented Nov 8, 2017

AIX failure is unrelated (and now fixed if you want to do a re-run but I don't think that's necessary).

@gireeshpunathil
Copy link
Member

gireeshpunathil pushed a commit that referenced this pull request Nov 13, 2017
PR-URL: #16827
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@gireeshpunathil
Copy link
Member

Landed in 8215c67 , thanks!

evanlucas pushed a commit that referenced this pull request Nov 13, 2017
PR-URL: #16827
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@evanlucas evanlucas mentioned this pull request Nov 13, 2017
MylesBorins pushed a commit that referenced this pull request Nov 17, 2017
PR-URL: #16827
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
addaleax pushed a commit that referenced this pull request Nov 18, 2017
Easier way to resolve conflicts from #16822 and #16827.

PR-URL: #17045
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@gibfahn gibfahn mentioned this pull request Nov 21, 2017
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
Easier way to resolve conflicts from #16822 and #16827.

PR-URL: #17045
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
gibfahn pushed a commit that referenced this pull request Dec 19, 2017
Easier way to resolve conflicts from #16822 and #16827.

PR-URL: #17045
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
Easier way to resolve conflicts from #16822 and #16827.

PR-URL: #17045
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants