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

Make tests more clear #8999

Closed
wants to merge 8 commits into from
Closed

Make tests more clear #8999

wants to merge 8 commits into from

Conversation

jennabelle
Copy link
Contributor

Checklist
  • [ x ] commit message follows commit guidelines
Affected core subsystem(s)

tests

Description of change

change 'var' to 'const' for clarity

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 10, 2016
@mscdex mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Oct 10, 2016
@gibfahn
Copy link
Member

gibfahn commented Oct 10, 2016

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

LGTM

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. Left a bunch of nits, but they aren't necessary for this to land. Would simply prefer them.

const common = require('../common');
const assert = require('assert');
const path = require('path');
const fs = require('fs');

var filename = path.join(common.fixturesDir, 'does_not_exist.txt');
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Since you're in here already, can you make filename a constant too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

const common = require('../common');
const assert = require('assert');
const path = require('path');
const fs = require('fs');

var filename = path.join(common.fixturesDir, 'does_not_exist.txt');
fs.readFile(filename, 'latin1', common.mustCall(function(err, content) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Can we add a line after assert.ok(err) like assert.strictEqual(err.code, 'ENOENT') or whatever the right error code is? I assume it will be the same across platform, but correction welcome.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

var path = require('path');
var fs = require('fs');
const path = require('path');
const fs = require('fs');


var filepath = path.join(common.tmpDir, 'write.txt');
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Can we make this filepath a constant too?

Copy link
Member

Choose a reason for hiding this comment

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

Nit: I think the var in line 84 can also be a const, no?

Copy link
Member

Choose a reason for hiding this comment

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

Nit: While we're in here refactoring anyway, maybe line 79 can be converted from assert.ok(err.message.indexOf('write after end') >= 0); to something more like assert.ok(err.message.includes('write after end'))?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@Trott
Copy link
Member

Trott commented Oct 11, 2016

LGTM if CI is OK.

jennabelle and others added 4 commits October 10, 2016 23:46
Add `assert.strictEqual(err.code, 'ENOENT')` and changed the last `var` to `const` for clarity
Refactor line 79 to use `includes()` instead of `indexOf() >= 0` for better readability. Change `var` to `const` for clarity
@jennabelle
Copy link
Contributor Author

Thx for all the tips! @Trott

@Trott
Copy link
Member

Trott commented Oct 11, 2016

Copy link
Member

@fhinkel fhinkel left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM.

Trott pushed a commit to Trott/io.js that referenced this pull request Oct 13, 2016
* var to const
* add check that expected error is ENOENT
* indexOf() to includes()

PR-URL: nodejs#8999
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@Trott
Copy link
Member

Trott commented Oct 13, 2016

Landed in a7970c0. Thanks!

@Trott Trott closed this Oct 13, 2016
jasnell pushed a commit that referenced this pull request Oct 14, 2016
* var to const
* add check that expected error is ENOENT
* indexOf() to includes()

PR-URL: #8999
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 11, 2016
* var to const
* add check that expected error is ENOENT
* indexOf() to includes()

PR-URL: #8999
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Nov 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

10 participants