test: improve test-fs-open-flags by vinimdocarmo · Pull Request #10908 · nodejs/node

vinimdocarmo · 2017-01-20T02:32:22Z

use arrow funcion instead of function expression
add second argumento to method assert.throws
check error messages from beginning to the end using ^ and $

Checklist

make -j4 test (UNIX)
commit message follows commit guidelines

Affected core subsystem(s)

test

* use arrow funcion instead of function expression * add second argument to method assert.throws * check error messages from beginning to the end using ^ and $

edsadr

Minor changes but looks good =)

edsadr · 2017-01-20T03:05:00Z

test/parallel/test-fs-open-flags.js

 );

 assert.throws(
  () => stringToFlags(null),


vinimdocarmo · 2017-01-20T04:08:37Z

@edsadr, I only added new lines between lines 40 and 43. Should I update all the lines pointed out by you?

Trott · 2017-01-20T05:44:29Z

I only added new lines between lines 40 and 43. Should I update all the lines pointed out by you?

I think my preference would be that you leave it as you currently have it, keeping the style of the existing file with the first argument on a line by itself. If you move all the args to the same line as the function call's opening parenthesis, then the linter will make you align all subsequent arguments, resulting in more churn. An then there will be yet more churn if that additional indentation causes a line to extend beyond 80 characters. So I think I actually would rather you keep it just like you already have it.

That said, I don't feel strongly about it, so if @edsadr or anyone else has compelling reasons for wanting those changes, I'm OK with it.

Trott

LGTM if CI is green

Trott · 2017-01-20T05:46:08Z

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

vinimdocarmo · 2017-01-20T10:07:02Z

@Trott, can you give me a feedback about the CI?

edsadr

@vinimdocarmo the CI is green, I was concerned about identation but as @Trott explains this could lead to more linting problems, then is fine and then.. LGTM 👍🏽

vinimdocarmo · 2017-01-20T13:12:18Z

Great! Thanks, @Trott @edsadr. Just waiting to see whether this PR lands or not.

Trott · 2017-01-20T20:04:55Z

@vinimdocarmo PRs stay open for at least 48 hours and that goes up to 72 hours if it stretches into a weekend like this one will. (Fell free to leave a "bump" comment if it's still open on Monday.)

vinimdocarmo · 2017-01-20T21:14:02Z

Okay, @Trott.

* use arrow funcion instead of function expression * add second argument to method assert.throws * check error messages from beginning to the end using ^ and $ PR-URL: #10908 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Adrian Estrada <edsadr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

edsadr · 2017-01-22T19:45:45Z

Landed in a024104, thanks for your contribution @vinimdocarmo

* use arrow funcion instead of function expression * add second argument to method assert.throws * check error messages from beginning to the end using ^ and $ PR-URL: nodejs#10908 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Adrian Estrada <edsadr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

* use arrow funcion instead of function expression * add second argument to method assert.throws * check error messages from beginning to the end using ^ and $ PR-URL: #10908 Backport-PR-URL: #13785 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Adrian Estrada <edsadr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

test: improve test-fs-open-flags

cd81e86

* use arrow funcion instead of function expression * add second argument to method assert.throws * check error messages from beginning to the end using ^ and $

nodejs-github-bot added the test Issues and PRs related to the tests. label Jan 20, 2017

edsadr requested changes Jan 20, 2017

View reviewed changes

test/parallel/test-fs-open-flags.js

);

assert.throws(

() => stringToFlags(null),

Copy link

Member

edsadr Jan 20, 2017

Choose a reason for hiding this comment

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

same here

mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Jan 20, 2017

Trott approved these changes Jan 20, 2017

View reviewed changes

edsadr approved these changes Jan 20, 2017

View reviewed changes

cjihrig approved these changes Jan 20, 2017

View reviewed changes

lpinca approved these changes Jan 21, 2017

View reviewed changes

targos approved these changes Jan 22, 2017

View reviewed changes

edsadr closed this Jan 22, 2017

vinimdocarmo deleted the test-updates branch January 22, 2017 21:51

MylesBorins added dont-land-on-v4.x labels Mar 8, 2017

targos mentioned this pull request Jun 19, 2017

(v6.x backport) tools: enforce two arguments in assert.throws #13785

Closed

2 tasks

gibfahn added backport-requested-v6.x and removed dont-land-on-v6.x labels Jun 19, 2017

MylesBorins added backported-to-v6.x and removed backport-requested-v6.x labels Jun 22, 2017

MylesBorins mentioned this pull request Jul 18, 2017

v6.11.2 proposal #14356

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

test: improve test-fs-open-flags#10908

test: improve test-fs-open-flags#10908
vinimdocarmo wants to merge 1 commit intonodejs:masterfrom
vinimdocarmo:test-updates

vinimdocarmo commented Jan 20, 2017

Uh oh!

edsadr left a comment

Uh oh!

edsadr Jan 20, 2017

Uh oh!

vinimdocarmo commented Jan 20, 2017 •

edited

Loading

Uh oh!

Trott commented Jan 20, 2017

Uh oh!

Trott left a comment

Uh oh!

Trott commented Jan 20, 2017

Uh oh!

vinimdocarmo commented Jan 20, 2017

Uh oh!

edsadr left a comment

Uh oh!

vinimdocarmo commented Jan 20, 2017 •

edited

Loading

Uh oh!

Trott commented Jan 20, 2017

Uh oh!

vinimdocarmo commented Jan 20, 2017

Uh oh!

edsadr commented Jan 22, 2017 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

Uh oh!

Conversation

vinimdocarmo commented Jan 20, 2017

Checklist

Affected core subsystem(s)

Uh oh!

edsadr left a comment

Choose a reason for hiding this comment

Uh oh!

edsadr Jan 20, 2017

Choose a reason for hiding this comment

Uh oh!

vinimdocarmo commented Jan 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Jan 20, 2017

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Jan 20, 2017

Uh oh!

vinimdocarmo commented Jan 20, 2017

Uh oh!

edsadr left a comment

Choose a reason for hiding this comment

Uh oh!

vinimdocarmo commented Jan 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Jan 20, 2017

Uh oh!

vinimdocarmo commented Jan 20, 2017

Uh oh!

edsadr commented Jan 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

vinimdocarmo commented Jan 20, 2017 •

edited

Loading

vinimdocarmo commented Jan 20, 2017 •

edited

Loading

edsadr commented Jan 22, 2017 •

edited

Loading